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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
