Cleaning up some warnings for Clang and GCC #1255
Conversation
`detach_abi(com_array<T>&)` uses two `memset` calls to zero two objects, one is an `std::pair` (from [microsoft#1165]), the other is a `winrt::com_array` (to clear its data pointer to prevent its destructor from freeing the array). GCC rightfully warns about this because these types are not trivially copyable, so calling memset on it is UB. This change fixes the UB and the GCC warning by reverting the first `memset` back to using the `std::pair` constructor as before [microsoft#1165], and changing the second `memset` to setting the member fields directly. Since this requires access to protected members, the function body is moved into a private member function, then the `winrt::detach_abi` free function is made `friend` of `com_array` to allow it to call the member function. This fix is not applied when `_MSC_VER` is defined in order to preserve the current behaviour on MSVC, in case [microsoft#1165] is still relevant. [microsoft#1165]: microsoft#1165
| result.second = *reinterpret_cast<impl::arg_out<T>*>(this); | ||
| memset(this, 0, sizeof(com_array<T>)); | ||
| #else | ||
| std::pair<uint32_t, impl::arg_out<T>> result(this->size(), *reinterpret_cast<impl::arg_out<T>*>(this)); |
There was a problem hiding this comment.
Doesn't this reintroduce the uninitialized padding data for non-VC compilers, per the comments in the PR linked above? What compiler warning does this resolve?
There was a problem hiding this comment.
The GCC warning is 'void* memset(void*, int, size_t)' clearing an object of type 'struct std::pair<unsigned int, float*>' with no trivial copy-assignment; use assignment or value-initialization instead [-Wclass-memaccess].
This change does leave the padding uninitialized, but as said before it is "almost never going to be an issue for this function" and I am uncomfortable with the attempt to fix it potentially invoking UB. (Though I am no language lawyer so I cannot say for sure whether there is UB.)
| @@ -418,14 +438,9 @@ WINRT_EXPORT namespace winrt | |||
| } | |||
|
|
|||
| template <typename T> | |||
| auto detach_abi(com_array<T>& object) noexcept | |||
| std::pair<uint32_t, impl::arg_out<T>> detach_abi(com_array<T>& object) noexcept | |||
There was a problem hiding this comment.
Same comment as above. This change undoes an intentional change made to address a customer-reported issue.
There was a problem hiding this comment.
Do you recall the issue? We should have a test to avoid such a regression, but all tests pass on this PR.
There was a problem hiding this comment.
Oh, you're referring to #1165 - that was itself to suppress a compiler warning. 🙄
There was a problem hiding this comment.
Ideally, we can avoid the memset in #1165 and just suppress the code analysis warning but I don't know whether that's possible since its presumably not an actual compiler warning. Perhaps @DefaultRyan can chime in.
There was a problem hiding this comment.
It looks to me like the original change was to suppress a warning in MSVC and this change is to suppress a warning in GCC. So if nobody objects, I'll go ahead and approve this change.
Fixed or suppressed some warnings in the headers when using Clang and GCC. The last commit ("gcc: Fix -Wclass-memaccess warning") involved a small refactoring.