Quuxplusone marked an inline comment as done. Quuxplusone 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 ---------------- 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 ================ Comment at: include/clang/AST/DeclCXX.h:482 + /// and a defaulted destructor. + unsigned IsNaturallyTriviallyRelocatable : 1; + ---------------- 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. ================ Comment at: include/clang/Basic/Attr.td:2096 +def TriviallyRelocatable : InheritableAttr { + let Spellings = [CXX11<"", "trivially_relocatable", 200809>, + CXX11<"clang", "trivially_relocatable">]; ---------------- 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. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8207 +def err_attribute_missing_on_first_decl : Error< + "type %0 declared %1 after its first declaration">; ---------------- 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. ================ Comment at: lib/AST/Type.cpp:2234 +bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const { + QualType T = Context.getBaseElementType(*this); + if (T->isIncompleteType()) ---------------- 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? 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
