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

[Arthur O'Dwyer via Phabricator via cfe-commits]

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