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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added subscribers: aaron.ballman, erichkeane.
erichkeane added a comment.

You'll not be able to get the C++ spelling without it getting into the standard.  Additionally, it seems easy enough to calculate upon need, so storing it in the AST is a waste.  @aaron.ballman is likely another person who can review this.



================
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
----------------
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.


================
Comment at: include/clang/AST/DeclCXX.h:482
+    /// and a defaulted destructor.
+    unsigned IsNaturallyTriviallyRelocatable : 1;
+
----------------
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.


================
Comment at: include/clang/Basic/Attr.td:2096
+def TriviallyRelocatable : InheritableAttr {
+  let Spellings = [CXX11<"", "trivially_relocatable", 200809>,
+                   CXX11<"clang", "trivially_relocatable">];
----------------
This spelling is almost definitely not acceptable until it is in the standard.  Also, why specify that it was added in 2008?


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8207
 
+def err_attribute_missing_on_first_decl : Error<
+  "type %0 declared %1 after its first declaration">;
----------------
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.


================
Comment at: lib/AST/Type.cpp:2234
+bool QualType::isTriviallyRelocatableType(const ASTContext &Context) const {
+  QualType T = Context.getBaseElementType(*this);
+  if (T->isIncompleteType())
----------------
You likely want to canonicalize here.


================
Comment at: lib/Parse/ParseDeclCXX.cpp:3811
   case ParsedAttr::AT_CXX11NoReturn:
+  case ParsedAttr::AT_TriviallyRelocatable:
     return true;
----------------
A note for future reviewers, after the C++11 spelling is removed, this likely needs to go as well.


Repository:
  rC Clang

https://reviews.llvm.org/D50119