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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 2 inline comments as done.
ldionne added a comment.

[forgot to submit some comments last time I updated the patch]



================
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;
----------------
Quuxplusone wrote:
> 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));
> }
> ```
I'm actually going to defer solving this problem by adding `-Wno-class-memaccess` when running the tests with GCC, because I'd rather postpone making this decision than making the wrong call. We can investigate this warning separately without blocking this review.

This won't be a problem for users, since they should all be using `-isystem` anyway -- this is really just for us, and that's why I'd rather take the time to investigate this warning.


================
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; }
----------------
Quuxplusone wrote:
> Quuxplusone wrote:
> > 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.
> It turns out it's not that simple. :(
> I'm still hoping that we could do something like
> ```
> template <class _Tp, size_t _Sz>
> #ifndef _LIBCPP_HAS_NO_CONCEPTS
>   requires (_Sz < numeric_limits<ptrdiff_t>::max())
> #endif
> _LIBCPP_INLINE_VISIBILITY
> constexpr ptrdiff_t ssize(const _Tp (&)[_Sz]) noexcept { return _Sz; }
> ```
> but the first step is to get a regression test (working on that in D121154).
I'll keep the pragma as-is and I'll pick up D121154.


================
Comment at: libcxx/include/barrier:126-129
+    __barrier_base(ptrdiff_t __expected, _CompletionF __completion_func = _CompletionF())
             : __expected(__expected), __base(__construct_barrier_algorithm_base(this->__expected),
                                              &__destroy_barrier_algorithm_base),
+              __expected_adjustment(0), __completion(std::move(__completion_func)), __phase(0)
----------------
philnik wrote:
> Is this a drive-by or does any compiler complain here?
This was a `-Wshadow` warning. I decided to turn it off because there were many benign instances of it.


================
Comment at: libcxx/utils/libcxx/test/params.py:25
   '-Wno-atomic-alignment',
+  '-Wno-shadow',
 
----------------
philnik wrote:
> Why are you disabling `-Wshadow`? I'm pretty certain this will regress very fast.
Because it results in a lot of not very useful warnings. I'll try enabling it again and give you more details.


================
Comment at: libcxx/utils/libcxx/test/params.py:43
   '-Wunreachable-code',
   '-Wno-unused-local-typedef',
 ]
----------------
philnik wrote:
> This doesn't look like it should be here. Why do we disable this warning?
Last I checked, it resulted in a *ton* of warnings because we often use typedefs in the test suite just to check that a type can be formed. Imagine for example `typedef std::vector<int>::pointer Ptr` just to confirm that `std::vector<int>::pointer` is well-formed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120684