[PATCH] D86126: Fix issue 47160: "`llvm::is_trivially_copyable` -- static assertion failure after DR 1734"

[Casey Carter via Phabricator via llvm-commits]

CaseyCarter added a comment.

I'm not going to "request changes" since I don't want you to feel like I'm asking for a novel on trivial special member functions. I'm just going to "comment" and race out of the review before someone asks me to do work ;)



================
Comment at: llvm/include/llvm/Support/type_traits.h:132
 };
 
----------------
The diff is missing context; I think you forgot to pass `-U 99999` to `git diff`.


================
Comment at: llvm/include/llvm/Support/type_traits.h:142
+//
+// `llvm::is_trivially_copyable` checks for the following:
+//   * if `T x(declval<const T&>())` is well formed,
----------------
"`llvm::is_trivially_copyable<T>` for a class type `T` checks for the following:" would be more precise and avoid confusion about where `T` comes from.


================
Comment at: llvm/include/llvm/Support/type_traits.h:145
+//     then all of T's copy constructors must be trivial,
+//     and there must be at least one non-deleted copy constructor.
+//   * if `declval<T&>() = declval<const T&>()` is well formed,
----------------
If `T x(declval<const T&>())` is well-formed, then `std::is_destructible_v<T> && std::is_copy_constructible_v<T>` is `true`. That fact implies neither that `T` has a copy constructor - that syntax could invoke a constructor template - nor that any copy constructor of `T` is trivial - that syntax could invoke a non-trivial copy constructor. Ditto the next three bullets, but replacing "copy" with "move" and "constructor/constructible" with "assignment operator/assignment" as appropriate.

The foregoing is why `detail::trivial_helper` exists. `detail::trivial_helper` is the dirt-simple union template:
```c++
template<class T>
union trivial_helper {
    T t;
};
```
Which has some convenient properties thanks to the rules for implicitly-defined special member functions for unions. First, "A defaulted destructor for a class `X` is defined as deleted if ... `X` is a union-like class that has a variant member with a non-trivial destructor ..." (n4861 [class.dtor]/7). If `is_destructible_v<trivial_helper<T>>` is `true`, then it must be the case that `T` has a trivial destructor, so `is_destructible_v<T> && is_destructible_v<trivial_helper<T>> == is_trivially_destructible_v<T>`. We have implemented `is_trivially_destructible`.

Second, "A defaulted copy/move constructor for a class `X` is defined as deleted if `X` has ... a potentially constructed subobject type `M` (or array thereof) that cannot be copied/moved ... [or] a variant member whose corresponding constructor as selected by overload resolution is non-trivial [or] any potentially constructed subobject of a type with a destructor that is deleted or inaccessible [or] for the copy constructor, a non-static data member of rvalue reference type." (n4861 [class.copy.ctor]/10). Tells us that `is_copy_constructible_v<trivial_helper<T>>` is roughly `is_trivially_copy_constructible_v<T> || !is_destructible_v<T>`. Since we have access to `is_destructible` we have an implementation of `is_trivially_copy_constructible`. Similarly, `is_move_constructible_v<trivial_helper<T>> == is_trivially_move_constructible_v<T> || !is_destructible_v<T>` which we can use to implement `is_trivially_move_constructible`.

Okay, we're rocketing along here - what about copy/move assignments? "A defaulted copy/move assignment operator for class `X` is defined as deleted if `X` has ... a variant member with a non-trivial corresponding assignment operator and `X` is a union-like class, or ... a direct non-static data member of class type `M` (or array thereof) ... that cannot be copied/moved" (n4861 [class.copy.assign]/7). similarly to the foregoing `is_copy_assignable_v<trivial_helper<T>>` and `is_move_assignable_v<trivial_helper<T>>` imply `is_trivially_copy_assignable_v<T>` and `is_trivially_move_assignable_v<T>`.

There's an additional piece of information we need to be aware of: the only way to have trivial destruction or copy/move construction/assignment is to have a trivial destructor or trivial copy/move constructor/assignment operator. There are non-special-member-function signatures that could intercept the syntax for these operations, but those other function / function templates would necessarily be deleted or non-trivial.

Given the foregoing, it's tempting to define `is_trivially_copyable_v<T>` as:
```c++
template <class T>
constexpr bool is_trivially_copyable_v = is_trivially_destructible_v<T> &&
    (is_trivially_copy_constructible_v<T> || !is_copy_constructible_v<T>) &&
    (is_trivially_move_constructible_v<T> || !is_move_constructible_v<T>) &&
    (is_trivially_copy_assignable_v<T> || !is_copy_assignable_v<T>) &&
    (is_trivially_move_assignable_v<T> || !is_move_assignable_v<T>) &&
    (is_trivially_copy_constructible_v<T> || is_trivially_move_constructible_v<T> ||
     is_trivially_copy_assignable_v<T> || is_trivially_move_assignable_v<T>);
```
where the `is_trivially_meowable` traits are implemented as developed above. This would be correct for a great many types, but there's a hidden premise: the preceding assumes that `is_meow<T> && !is_trivially_meow<T>` implies that `T`'s meow (copy/move constructor/assignment operator) is not deleted. It is, however, possible to have a type with a deleted special member function for which the corresponding syntax invokes a non-special member function. Case in point, MSVC's `std::pair` looks a lot like:
```c++
template <class F, class S>
struct pair {
  F first;
  S second;

  ~pair() = default;
  pair(const pair&) = default;
  pair(pair&&) = default;
  // ...

  pair& operator=(const volatile pair&) = delete;
  template <enable_if_t</*...*/, int> = 0>
  pair& operator=(const pair&) {
    // ...
  }

  template <enable_if_t</*...*/, int> = 0>
  pair& operator=(pair&&) {
    // ...
  }
};
```
If both `F` and `S` are trivially destructible, trivially move constructible, and trivially copy constructible (or not copy constructible) then `pair<F, S>` is trivially copyable regardless of `is_copy_assignable` and `is_move_assignable`.

In any case, the definition this file uses is actually:
```c++
template <class T>
constexpr bool is_trivially_copyable_v = is_trivially_destructible_v<T> &&
    (is_trivially_copy_constructible_v<T> || !is_copy_constructible_v<T>) &&
    (is_trivially_move_constructible_v<T> || !is_move_constructible_v<T>) &&
    (is_trivially_copy_assignable_v<T> || !is_copy_assignable_v<T>) &&
    (is_trivially_move_assignable_v<T> || !is_move_assignable_v<T>);
```
which corresponds to the C++14 (pre-CWG 1734) formulation of trivial copyability and not the C++17 formulation. (Clang implements `__is_trivially_copyable` similarly, having not yet implemented CWG 1734.) The best fix would be to avoid this fillin trait completely on platforms that provide it in the STL - I think all supported compilers/standard libraries except for libstdc++ pre-5.0? - but a close second is to not expect the trait to agree with the STL.



================
Comment at: llvm/include/llvm/Support/type_traits.h:161
+// whereas `std::is_trivially_copyable` checks for:
+//   all of T's copy constructors, move constructors,
+//   copy assignment operators, and move assignment operators are
----------------
Since C++20 (Specifically P0848R3) "all of T's _eligible_ copy constructors, move constructors, ..." would be more precise. I acknowledge this may be more precision than you are interested in =)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86126