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

[Nicolas Lesser via Phabricator via cfe-commits]

Rakete1111 added a reviewer: Rakete1111.
Rakete1111 added inline comments.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:6091
 
+  for (auto *F : Record->fields()) {
+    if (F->isMutable()) {
----------------
Quuxplusone wrote:
> 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.
Done. :) Thanks


================
Comment at: lib/Sema/SemaDeclCXX.cpp:6174
+      Record->hasAttr<TriviallyRelocatableAttr>() &&
+      !isTemplateInstantiation(Record->getTemplateSpecializationKind())) {
+    if (Record->getDefinition() && !Record->isDependentContext() &&
----------------
Quuxplusone wrote:
> 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?
Yes, good idea.


Repository:
  rC Clang

https://reviews.llvm.org/D50119