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

[Aaron Ballman via Phabricator via libcxx-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

The attribute parts LGTM, but I did have a question about the libc++ parts.



================
Comment at: clang/include/clang/Basic/Attr.td:2367
+def PreferredName : InheritableAttr {
+  let Spellings = [Clang<"preferred_name">];
+  let Subjects = SubjectList<[ClassTmpl]>;
----------------
rsmith wrote:
> 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?
You got the important points -- it changes the feature testing macro behavior and impacts diagnostics, but otherwise, not a whole lot of functional difference.


================
Comment at: clang/include/clang/Basic/Attr.td:2369
+  let Subjects = SubjectList<[ClassTmpl]>;
+  let Args = [TypeArgument<"TypedefType">];
+  let Documentation = [PreferredNameDocs];
----------------
rsmith wrote:
> 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.
> On balance I weakly prefer the one-type-per-attribute model, but I'm happy to let you make the call.

That all sounds like compelling rationale to me, so I'm fine with the current approach. The biggest benefit I can see to the other approach (aside from shorter code) is that it feels like it would be slightly faster to parse in these very commonly included header files, but that may wash out in the extra work to instantiate the template anyway.


================
Comment at: clang/include/clang/Basic/Attr.td:2372
+  let InheritEvenIfAlreadyPresent = 1;
+  let MeaningfulToClassTemplateDefinition = 1;
+  let TemplateDependent = 1;
----------------
rsmith wrote:
> 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.
Huh, that does seem backwards. I'll investigate and maybe swap the polarity if it seems sensible, thanks for mentioning it!


================
Comment at: libcxx/include/iosfwd:188
 
+#ifdef _LIBCPP_PREFERRED_NAME
+template <class _CharT, class _Traits>
----------------
We always define `_LIBCPP_PREFERRED_NAME` so is this actually needed?


================
Comment at: libcxx/include/iosfwd:245
 
+#ifdef _LIBCPP_PREFERRED_NAME
+template <class _CharT, class _Traits, class _Allocator>
----------------
Same question here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311