erichkeane added inline comments.
================ Comment at: docs/LanguageExtensions.rst:1093 library. +* ``__is_trivially_relocatable`` (Clang): Determines whether moving an object + of type ``type``, and then destroying the source object, is functionally ---------------- Quuxplusone wrote: > erichkeane wrote: > > How would this behave with unions? I don't see any exclusions happening on > > union-ness. A CXXRecordDecl can be a union as well as a class. > My intent is to make Clang's behavior match the wording in P1144R0; so, it > should work for unions as well as for structs/classes. Any union which is > trivially move-constructible and trivially destructible will be > `__is_trivially_relocatable`. Any union which is non-trivially destructible > *must* have a user-provided destructor, and therefore will *not* be > `__is_trivially_relocatable` unless the user has annotated it with the > attribute. > https://p1144.godbolt.org/z/F06TTQ Makes sense. ================ Comment at: include/clang/AST/DeclCXX.h:482 + /// and a defaulted destructor. + unsigned IsNaturallyTriviallyRelocatable : 1; + ---------------- Quuxplusone wrote: > erichkeane wrote: > > Typically we'd have this calculated when requested rather than stored. I > > suspect using a bit for something like this isn't going to be terribly > > acceptable. > You know better than I do; but I'm also not sure how to calculate it on > request. You'll end up just recursively walking the CXXRecordDecl upon application of the type trait, and need to check the things that make it NOT trivially relocatable. I think it also extensively simplifies this patch, since none of the CXXRecordDecl functions are required (just the type-trait emission). ================ Comment at: include/clang/Basic/Attr.td:2096 +def TriviallyRelocatable : InheritableAttr { + let Spellings = [CXX11<"", "trivially_relocatable", 200809>, + CXX11<"clang", "trivially_relocatable">]; ---------------- Quuxplusone wrote: > erichkeane wrote: > > This spelling is almost definitely not acceptable until it is in the > > standard. Also, why specify that it was added in 2008? > Agreed, it should be `[[clang::trivially_relocatable]]` for Clang's purposes. > This spelling was because this patch came from the Godbolt Compiler Explorer > patch where I wanted the shorter/future spelling for public relations > reasons. :) > IIUC, the appropriate fix here is to change these two lines from > ``` > let Spellings = [CXX11<"", "trivially_relocatable", "200809">, > CXX11<"clang", "trivially_relocatable">]; > ``` > to > ``` > let Spellings = [Clang<"trivially_relocatable">, > ``` > I believe the "200809" was because I wanted it to be available in C++11 mode > on Godbolt. Yep, thats the suggested fix. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8207 +def err_attribute_missing_on_first_decl : Error< + "type %0 declared %1 after its first declaration">; ---------------- Quuxplusone wrote: > erichkeane wrote: > > I'm shocked that there isn't a different diagnostic to do this same thing. > > @aaron.ballman likely knows better... I haven't seen the usage yet, but I > > presume you don't necessarily want a behavior that doesn't allow forward > > declarations. > I would be very happy to see this diagnostic get into trunk separately and > earlier than D50119. There are some other diagnostics that could be merged > with this one, e.g. `[[noreturn]]` needs a version of this diagnostic, and I > believe `[[clang::trivial_abi]]` should have it added. > > I don't know how to link to comments on Phabricator, but Ctrl+F downward for > this example: > ``` > struct S { S(S&&); ~S(); }; > std::vector<S> vec; > struct [[trivially_relocatable]] S; // ha ha, now you have to re-do all of > vector's codegen! > ``` > This is why it is important to diagnose and disallow "backward declarations." > I don't particularly care about "forward declarations" (either to allow or > disallow them). The attribute would end up getting used only on library types > where IMHO nobody should ever be forward-declaring them anyway. E.g. it is > not a good idea for a regular C++ programmer to forward-declare `unique_ptr`. > But if there's a way to allow forward-declarations (when the type remains > incomplete) while disallowing backward-declarations (adding the attribute to > an already-complete type), then I will be happy to do it. Sure, I get that... I'm just surprised/amazed it doesn't already exist. Hopefully Aaron takes a look. ================ Comment at: lib/AST/Type.cpp:2234 +bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const { + QualType T = Context.getBaseElementType(*this); + if (T->isIncompleteType()) ---------------- Quuxplusone wrote: > erichkeane wrote: > > You likely want to canonicalize here. > You mean `QualType T = Context.getBaseElementType(getCanonicalType());`? > I can do that. For my own edification (and/or a test case), in what way does > the current code fail? More like: QualType T = Context.getBaseElementType(*this).getCanonicalType(); This desugars typedefs in some cases, which could make the below fail. Something like this: https://godbolt.org/z/-C-Onh It MIGHT still pass here, but something else might canonicalize this along the way. ADDITIONALLY, I wonder if this properly handles references? I don't think getBaseElementType will de-reference. You might want to do that as well. 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
