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

[Erich Keane via Phabricator via cfe-commits]

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