[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:
> 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.


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

https://reviews.llvm.org/D54472