[libcxx-commits] [PATCH] D120684: [libc++] Define _LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER whenever we enable warnings in the test suite

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__algorithm/copy.h:56
     if (__n > 0)
-        _VSTD::memmove(__result, __first, __n * sizeof(_Up));
+        _VSTD::memmove(static_cast<void*>(__result), static_cast<void const*>(__first), __n * sizeof(_Up));
     return __result + __n;
----------------
ldionne wrote:
> Quuxplusone wrote:
> > Here and throughout: what is the compiler complaining about if you don't do this? Implicit conversion from `T*` to `void*` is absolutely part of C++ since forever, until forever; and this isn't an ADL situation either; so I can't imagine what a compiler could possibly see to warn about here.
> It is complaining that we're using `memmove` (or `memcpy`) on a type that doesn't have a trivial copy constructor or copy assignment operator. Above, we check for `is_trivially_copy_assignable`, which means that we can sometimes call this function on a type that has a trivial copy assignment, but not a trivial copy constructor.
> 
> The same goes for `__construct_range_forward`, but there instead we are calling `memcpy` on a type that has a trivial copy constructor, but no trivial copy assignment operator.
> 
> I'm not sure why GCC sees fit to warn about this. LMK if you think another approach is better to silence the warnings.
We //could// fix this by adding `-Wno-class-memaccess` to the tests.
On the one hand, `-Wno-class-memaccess` feels more philosophically honest; just adding a cast to `void*` doesn't make the UB go away, and the wording of GCC's warning message doesn't imply that they think adding the cast //should// shut up the warning. We might find that in GCC n+1, the compiler can see through the cast and warns anyway.
https://godbolt.org/z/K5Ex3hxP6
```
warning: 'void* memcpy(void*, const void*, size_t)' writing to an object
of non-trivially copyable type 'struct A'; use copy-assignment or
copy-initialization instead [-Wclass-memaccess]
```
On the other hand, passing `-Wno-class-memaccess` to our own tests doesn't fix the problem for anybody else... if there even //is// a problem for anyone else. We're moving to `-I` for the tests, but surely we expect actual //users// to be using `-isystem` and so they'll never care about this warning, right?
Also, passing `-Wno-class-memaccess` to the tests would fail to alert us if anywhere //unintentionally// ran foul of this warning.

If this were my own codebase, my preferences might be
(1) `-Wno-class-memaccess` globally, or
(2) `#define _LIBCPP_MEMMOVE(x, y, n) std::memmove((void*)x, (const void*)y, n)` and use that in the specific places where the GCC warning would come up; ditto presumably `_LIBCPP_MEMCPY`.
But (2) kinda sucks.

How about just doing what you're doing (cast to `void*`), but add a comment on the line above, like
```
if (__n > 0) {
    // Cast the arguments to silence GCC's -Wclass-memaccess
    std::memmove(static_cast<void*>(__result), static_cast<void const*>(__first), __n * sizeof(_Up));
}
```


================
Comment at: libcxx/include/__iterator/size.h:44-51
+// GCC complains about the implicit conversion from ptrdiff_t to size_t in
+// the array bound.
+_LIBCPP_DIAGNOSTIC_PUSH
+_LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wsign-conversion")
 template <class _Tp, ptrdiff_t _Sz>
 _LIBCPP_INLINE_VISIBILITY
 constexpr ptrdiff_t ssize(const _Tp (&)[_Sz]) noexcept { return _Sz; }
----------------
I didn't notice this before. Isn't this just a "bug" on our and/or the standard's part? Shouldn't this be
```
template <class _Tp, size_t _Sz>
_LIBCPP_INLINE_VISIBILITY
constexpr ptrdiff_t ssize(const _Tp (&)[_Sz]) noexcept { return static_cast<ptrdiff_t>(_Sz); }
```
It is currently specified to take a `ptrdiff_t _Sz` parameter, but that's surprising, and if it causes compiler warnings, then we should just file an LWG issue to get it changed.
https://eel.is/c++draft/iterator.range#lib:ssize(T_(&array)%5bN%5d)
I'll send an email.

I think we should just preemptively change our `_Sz`'s type to suppress the warning. I don't think the change is detectable by a conforming program.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120684/new/

https://reviews.llvm.org/D120684