[PATCH] D54472: Disable invalid isPodLike<> specialization

[serge via Phabricator via llvm-commits]

serge-sans-paille marked an inline comment as done.
serge-sans-paille added inline comments.


================
Comment at: include/llvm/Support/type_traits.h:163
+      (has_trivial_copy_constructor || has_trivial_copy_assign ||
+       has_trivial_move_constructor || has_trivial_move_assign);
+
----------------
Quuxplusone wrote:
> serge-sans-paille wrote:
> > serge-sans-paille wrote:
> > > Quuxplusone wrote:
> > > > This last clause — `(has_trivial_copy_constructor || has_trivial_copy_assign || has_trivial_move_constructor || has_trivial_move_assign)` — does not correspond to anything in the Standard. So:
> > > > 
> > > > https://godbolt.org/z/R5hCff
> > > > ```
> > > > struct S {
> > > >     S(S&&) = delete;
> > > > };
> > > > static_assert(is_trivially_copyable<S>::value);
> > > > ```
> > > > ```
> > > > error: static_assert failed "consistent behavior"
> > > >   static_assert(value == std::is_trivially_copyable<T>::value, "consistent behavior");
> > > >   ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > ```
> > > > I'm not sure but I would suggest just getting rid of this last clause.
> > > From https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable (yeah that's not the standard, but I do hope it's derived from the standard)
> > > 
> > > > at least one copy constructor, move constructor, copy assignment operator, or move assignment operator is non-deleted
> > > 
> > > That's the intent I was trying to capture.
> > > 
> > > I'm okay with getting rid of this capture, especially when looking at https://godbolt.org/z/p3-Cy4.
> > @Quuxplusone I had to reintroduce the ` (has_trivial_move_assign || has_trivial_move_constructor || has_trivial_copy_assign || has_trivial_copy_constructor)` condition, which is indeed a standard requirement. It basically states « if the class is not meant to be copied, don't allow to copy it ».
> The standard trait "trivially copyable" is not related to copy-{constructibility,assignability}; it's its own thing.
> 
> I insist that if you're naming your thing "trivially copyable," it should exactly match the Standard trait's behavior, i.e., it should succeed at this test case:
> 
> ```
> struct S {
>     S(S&&) = delete;
> };
> static_assert(is_trivially_copyable<S>::value);
> ```
> 
> Your current code doesn't pass that test. https://godbolt.org/z/j6pkaS
> 
> Any users of `is_trivially_copyable` should use it only //to enable perf optimizations//. They shouldn't be using it to ask, "//Should// I make a copy of this?" First of all, "make a copy" is underspecified. What they should have asked is "Should I copy-construct this?" or "Should I copy-assign this?" And we already have traits for that. Then, once they've already decided to make a copy (or maybe not to make any copies, but just to //move// the value without copying), //then// they should look at `is_trivially_copyable` to decide //how// to make that copy.
I totally agree on your point. Unfortunately, I don't think it's possible to achieve a portable implementation of is_trivially_copyable without compiler support for all supported compiler version (including gcc 4.9). And that's exactly why this patch exist: to cope with this limitation, while ensuring the memcpy are valid, which is not the case with current implementation (which is both invalid for some type/compiler combination, and not conservative with respect to ABI because of the impact of `isPodLike` on ADT like `SmallVector`. So I would advocate for renaming this trait `is_memc(o)pyable` and ensuring that forall T instanciated in LLVM where `std::is_trivially_copyable` matters,  `is_memcopyable<T>::value == true` implies `std::is_trivially_copyable<T> == true`, which is currently the case (as we actually even verify that  `is_memcopyable<T>::value == std::is_trivially_copyable<T>::value`).

Does that sound OK to you?


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

https://reviews.llvm.org/D54472