[PATCH] D54472: Disable invalid isPodLike<> specialization

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

Quuxplusone 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);
+
----------------
serge-sans-paille wrote:
> 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?
I think that `is_memcpyable` sounds underdetermined: it ought to be `is_move_assignable_via_memcpy`, `is_copy_constructible_via_memcpy`, or whatever operation we're asserting is tantamount to memcpy. At which point you've just reinvented `is_trivially_move_assignable`, `is_trivially_copy_constructible` etc. (and here I'll insert a shameless plug for D50119 `is_trivially_relocatable`, because quite often the operation we want to replace is "relocating", not merely "copy-constructing" or "move-assigning").

The situation is complicated by the fact that right now the C++17 standard specifies that memcpying is technically permitted only for trivially copyable types. So if by `is_memcpyable` you mean "is memcpying technically permitted for this type under //any// circumstances — never mind whether it's semantically correct —" then `is_memcpyable` would be just a synonym for `std::is_trivially_copyable`. (But then specializing it would be UB.)

I think this patch has been evolving toward "Provide an `llvm::is_trivially_copyable` trait that is exactly the same as `std::is_trivially_copyable`, but portable." I think it should keep going in that direction.

If we say "Maybe the trait shouldn't be `is_trivially_copyable` at all, but something else," then that reopens the can of worms. (At that point, why not just keep the name `isPodLike`?)


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

https://reviews.llvm.org/D54472