[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

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

Quuxplusone marked an inline comment as done.
Quuxplusone added inline comments.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:6091
 
+  for (auto *F : Record->fields()) {
+    if (F->isMutable()) {
----------------
Rakete1111 wrote:
> Can you move this in `ActOnFields`? That way we avoid two iterations of the fields.
Done. Btw, I notice that `ActOnFields` spends a lot of time doing `dyn_cast<CXXRecordDecl>(Record)` over and over. If I had commit privs, I'd refactor it to compute `CXXRecordDecl *CXXRecord = dyn_cast_or_null<CXXRecordDecl>(Record);` once at the very top of the function, and then use `CXXRecord` throughout.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:6174
+      Record->hasAttr<TriviallyRelocatableAttr>() &&
+      !isTemplateInstantiation(Record->getTemplateSpecializationKind())) {
+    if (Record->getDefinition() && !Record->isDependentContext() &&
----------------
Rakete1111 wrote:
> The call to `isTemplateInstantiation` is wrong. Consider:
> 
> ```
> template<class T>
> struct [[trivially_relocatable]] A {
>   T t;
> };
> 
> struct X {
>   X() = default;
>   X(X &&) = delete;
> };
> 
> A<X> d;
> static_assert(!__is_trivially_relocatable(decltype(d))); // oops, fires
> ```
> 
> There is also no diagnostic saying that `A<X>` cannot be marked `[[trivially_relocatable]]`.
The absence of any diagnostics is intentional. We're saying that `A` is trivially relocatable except-of-course-when-it's-not-relocatable-at-all, similar to how templates currently work with `constexpr`.

However, the fact that `__is_trivially_relocatable(A<X>)` when `!__is_constructible(A<X>, A<X>&&)` does seem like a bug. I should probably move the `isTemplateInstantiation` check down to control just the diagnostic, eh?


================
Comment at: lib/Sema/SemaDeclCXX.cpp:6176
+    if (Record->getDefinition() && !Record->isDependentContext() &&
+        !Record->isBeingDefined()) {
+      // Check that the destructor is non-deleted.
----------------
Rakete1111 wrote:
> `Record` is never being defined at this point, even for templates. It also always has a definition AFAIK.
Yes, that matches my observations so far (but I'm running the tests again to confirm). I'd originally copied this formula from somewhere else, I forget where.


Repository:
  rC Clang

https://reviews.llvm.org/D50119