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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/Attr.td:2096
+def TriviallyRelocatable : InheritableAttr {
+  let Spellings = [CXX11<"", "trivially_relocatable", 200809>,
+                   CXX11<"clang", "trivially_relocatable">];
----------------
erichkeane wrote:
> 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.
> to
> `let Spellings = [Clang<"trivially_relocatable">,`

Close, but since this attribute has no meaning in C mode, it should be `let Spellings = [Clang<"trivially_relocatable", 0>,`


================
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:
> 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.
Yeah, I'm also surprised this doesn't already exist, but... it doesn't seem to. We have `err_noreturn_missing_on_first_decl`, `err_carries_dependency_missing_on_first_decl`, and `err_internal_linkage_redeclaration` to all do the same thing.


Repository:
  rC Clang

https://reviews.llvm.org/D50119