[libcxx-commits] [PATCH] D91311: Add new 'preferred_name' attribute.

[Aaron Ballman via Phabricator via libcxx-commits]

aaron.ballman added a comment.

I really like this attribute, thank you for working on this!



================
Comment at: clang/include/clang/Basic/Attr.td:2367
+def PreferredName : InheritableAttr {
+  let Spellings = [Clang<"preferred_name">];
+  let Subjects = SubjectList<[ClassTmpl]>;
----------------
This seems like one we should exempt from C code, WDYT? If you agree, you can change it to `Clang<"preferred_name", /*AllowInC*/ 0>`


================
Comment at: clang/include/clang/Basic/Attr.td:2369
+  let Subjects = SubjectList<[ClassTmpl]>;
+  let Args = [TypeArgument<"TypedefType">];
+  let Documentation = [PreferredNameDocs];
----------------
Would it make sense for this to be a variadic parameter of type arguments (with a constraint that at least one type be named)? This way you can write: `[[clang::preferred_name(string, wstring)]]` instead of `[[clang::preferred_name(string), clang::preferred_name(wstring)]]`


================
Comment at: clang/include/clang/Basic/AttrDocs.td:4398
+
+The preferred name must be a typedef declaration that refers to a
+specialization of the class template. In general this requires the template to
----------------
May want to be clear that this includes alias declarations as well as typedefs (the example helps though). Also, you should mention that the typedef cannot include qualifiers.


================
Comment at: clang/lib/AST/TypePrinter.cpp:1353
+  // Print the preferred name if we have one for this type.
+  for (PreferredNameAttr *PNA :
+       T->getDecl()->specific_attrs<PreferredNameAttr>()) {
----------------
`const auto *`?


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:1394
+
+  if (!T->getAs<TypedefType>() || T.hasQualifiers()) {
+    S.Diag(AL.getLoc(), diag::err_attribute_preferred_name_arg_invalid)
----------------
Given that this is checking properties of the type used in the attribute, I wonder if it makes sense to note the original declaration. I left a note on a test case below about this.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:557
+  // template.
+  if (auto *PNA = dyn_cast<PreferredNameAttr>(A)) {
+    QualType T = PNA->getTypedefType();
----------------
Can you fix these two lint warnings?


================
Comment at: clang/test/SemaTemplate/attributes.cpp:79
+  template<typename T> struct [[clang::preferred_name(const X)]] C; // expected-error {{argument 'const preferred_name::X'}}
+  template<typename T> struct [[clang::preferred_name(Z)]] C; // expected-error {{argument 'preferred_name::Z' (aka 'const C<double>')}}
+  template<typename T> struct C {};
----------------
This looks like a pretty reasonable declaration but only fails because of an issue with the declaration of `Z` -- should we point out more explicitly that the qualifier on the alias declaration is the issue?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91311/new/

https://reviews.llvm.org/D91311