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

[Richard Smith - zygoloid via Phabricator via libcxx-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:2367
+def PreferredName : InheritableAttr {
+  let Spellings = [Clang<"preferred_name">];
+  let Subjects = SubjectList<[ClassTmpl]>;
----------------
aaron.ballman wrote:
> This seems like one we should exempt from C code, WDYT? If you agree, you can change it to `Clang<"preferred_name", /*AllowInC*/ 0>`
Done. For my information, what difference does this make? The `Subjects` list already means that it can't ever be applied in C -- does this effectively exclude it from `__has_c_attribute` (and maybe change the diagnostic on use), or does it go deeper than that?


================
Comment at: clang/include/clang/Basic/Attr.td:2369
+  let Subjects = SubjectList<[ClassTmpl]>;
+  let Args = [TypeArgument<"TypedefType">];
+  let Documentation = [PreferredNameDocs];
----------------
aaron.ballman wrote:
> 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)]]`
I did think a little bit about that, and I don't mind going in that direction. I have only very weak justifications for the current approach:

* I think we would want to allow the attribute to be repeated and accumulate regardless (see the `<iosfwd>` + `<string>` example for `std::basic_string` in libc++), and I would prefer to not have two different syntaxes for the same thing.
* The separate attributes give us some room for extensibility in the future, by adding new optional arguments, though I don't have a great example for such a possible extension.
* Implementation simplicity: when instantiating a template with `preferred_name(X)`, we either drop the attribute or retain it depending on whether the name is `X`, and if there were multiple names, we'd need to do something more sophisticated. (Also the trivial simplicity of only iterating over one list of types, rather than a list of lists.)
* I expect users of this attribute to be very rare (libc++, maybe a couple of types in Abseil, maybe a handful of types in boost, probably not much else), so optimizing for them may not be worth any added complexity, and I wouldn't expect any significant difference in parse time / memory usage / etc from the combined model versus the separate model.

On balance I weakly prefer the one-type-per-attribute model, but I'm happy to let you make the call.


================
Comment at: clang/include/clang/Basic/Attr.td:2372
+  let InheritEvenIfAlreadyPresent = 1;
+  let MeaningfulToClassTemplateDefinition = 1;
+  let TemplateDependent = 1;
----------------
Incidentally, this flag appears to be backwards? If set to `1`, we instantiate the attribute with the declaration; if set to `0` we instantiate only with the definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311