[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 7 inline comments as done.
ldionne 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;
----------------
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.


================
Comment at: libcxx/include/experimental/simd:1473-1474
 
+  _LIBCPP_DIAGNOSTIC_PUSH
+  _LIBCPP_GCC_DIAGNOSTIC_IGNORED("-Wnon-template-friend")
   // binary operators [simd.binary]
----------------
Quuxplusone wrote:
> Just from a quick glance at this code, I'm confused. Where are the definitions of these friend functions? If they don't actually exist, then perhaps these lines should just be removed, or at least `#if 0`'d out.
They are not defined anywhere. Our implementation of `experimental::simd` is not really complete. I'll `#if 0` them out.


================
Comment at: libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp:12-14
+// We voluntarily use std::default_initializable on types that have redundant
+// or ignored cv-qualifiers -- don't warn about it.
+// ADDITIONAL_COMPILE_FLAGS: -Wno-ignored-qualifiers
----------------
Quuxplusone wrote:
> I don't understand this comment.
GCC produces the following warning:

```
In file included from /llvm/build/generic-gcc/include/c++/v1/__concepts/arithmetic.h:13,
                 from /llvm/build/generic-gcc/include/c++/v1/concepts:132,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/incrementable_traits.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/concepts.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/distance.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__algorithm/equal.h:15,
                 from /llvm/build/generic-gcc/include/c++/v1/array:111,
                 from /llvm/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp:21:
/llvm/build/generic-gcc/include/c++/v1/type_traits: In instantiation of 'struct std::__1::is_constructible<void (Empty::* const)(const int&)>':
/llvm/build/generic-gcc/include/c++/v1/type_traits:2927:77:   required from 'constexpr const bool std::__1::is_constructible_v<void (Empty::* const)(const int&)>'
/llvm/build/generic-gcc/include/c++/v1/__concepts/constructible.h:28:26:   required from 'void test_not_const() [with T = void (Empty::*)(const int&)]'
/llvm/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp:192:62:   required from here
/llvm/build/generic-gcc/include/c++/v1/type_traits:2922:38: error: type qualifiers ignored on cast result type [-Werror=ignored-qualifiers]
 2922 |     : public integral_constant<bool, __is_constructible(_Tp, _Args...)>
      |                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /llvm/build/generic-gcc/include/c++/v1/concepts:138,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/incrementable_traits.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/concepts.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__iterator/distance.h:14,
                 from /llvm/build/generic-gcc/include/c++/v1/__algorithm/equal.h:15,
                 from /llvm/build/generic-gcc/include/c++/v1/array:111,
                 from /llvm/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp:21:
/llvm/build/generic-gcc/include/c++/v1/__concepts/constructible.h: In instantiation of 'void test_not_const() [with T = void (Empty::*)(const int&)]':
/llvm/libcxx/test/std/concepts/concepts.lang/concept.default.init/default_initializable.compile.pass.cpp:192:62:   required from here
/llvm/build/generic-gcc/include/c++/v1/__concepts/constructible.h:37:16: error: type qualifiers ignored on cast result type [-Werror=ignored-qualifiers]
   37 |     requires { _Tp{}; } && __default_initializable<_Tp>;
      |                ^~~~~
```

I believe the relevant bit here is that we're calling `std::is_constructible_v<void (Empty::* const)(const int&)>`, and GCC is basically saying that we don't care about the `const` qualifier in `Empty::* const`.



================
Comment at: libcxx/test/support/type_classification/movable.h:71-72
+  copy_with_mutable_parameter(copy_with_mutable_parameter&);
+  copy_with_mutable_parameter&
+  operator=(copy_with_mutable_parameter&);
 };
----------------
Quuxplusone wrote:
> I don't understand the relevance of these changes, but I certainly don't object.
Otherwise, GCC warns about deprecated implicit definition of the copy constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120684