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