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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
