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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/include/regex:2520
+    _LIBCPP_PREFERRED_NAME(wregex)
+    basic_regex
 {
----------------
aaron.ballman wrote:
> rsmith wrote:
> > Quuxplusone wrote:
> > > Why does this attribute go on the class template? Shouldn't it be an attribute on the typedef, so that you don't have to repeat yourself? I mean, I'd much rather see
> > > 
> > >     template<class T> class BasicFoo { };
> > >     using [[preferred]] Foo = BasicFoo<A>;
> > >     using [[preferred]] WFoo = BasicFoo<B>;
> > > 
> > > than
> > > 
> > >     template<class> class BasicFoo;
> > >     using Foo = BasicFoo<A>;
> > >     using WFoo = BasicFoo<B>;
> > >     template<class T> class [[preferred(Foo, WFoo)]] BasicFoo { };
> > > 
> > > The latter repeats every identifier one extra time, compared to the former.
> > > 
> > > And then, in fact, couldn't you go one step further and say that typedefs in the same scope as the class template itself should //always// implicitly have this attribute? Even if the attribute doesn't appear in the source code, we still want to print `basic_regex<char>` as `regex`, right? It shouldn't cost any additional work for the compiler to figure that out.
> > I don't think we want arbitrary typedefs to be able to declare themselves to be names for the class template after the fact: this is a decision that the (author of the) template itself should be making, not a decision for the (author of the) typedef. Also, we need the information when looking at the class template, not when looking at the typedef, so attaching it to the typedef would require us to internally attach it to the class template instead anyway, and generally it's undesirable and problematic to mutate an existing declaration -- it's much cleaner to introduce new properties of a declaration by redeclaring it. It's certainly not ideal that this results in extra forward declarations in some cases, but given that this attribute is only expected to be written a few dozen times in total, it doesn't seem worth worrying about too much.
> > 
> > I don't think we want typedefs to automatically get this behavior, even if they're in the same namespace. I think it would be problematic if, for example:
> > 
> > ```
> > namespace absl {
> >   template<typename T> struct basic_string_view;
> >   using format_arg = basic_string_view<char>;
> >   std::string format(format_arg, ...);
> > }
> > ```
> > 
> > ... resulted in `absl::string_view` sometimes displaying as the (to a user) unrelated type `absl::format_arg` depending on (presumably) `#include` order, and I don't think we want to be in the business of inventing heuristics to pick the "right" typedef.
> > I don't think we want arbitrary typedefs to be able to declare themselves to be names for the class template after the fact: this is a decision that the (author of the) template itself should be making, not a decision for the (author of the) typedef. 
> 
> +1
> 
> 
> generally it's undesirable and problematic to mutate an existing declaration -- it's much cleaner to introduce new properties of a declaration by redeclaring it

Well, I can't argue with that one. :)

I see your point about the author of the typedef vs. the author of the template, but actually I'm ambivalent about your `format_arg` example. I could vaguely imagine that someone might want to see `absl::format_arg` in an error message, in the context of a call to `absl::format`, at least. But, I can see the point that hypotheticals aren't relevant; this attribute is specifically tailored for the STL's idiom of "class template with a bunch of closely associated typedefs," and the STL is monolithic enough that the class template always knows the exact names of those typedefs (I mean, in the STL, it's reasonable for a human to say that `basic_string_view` //is// `string_view`, in a way that it //is// not `format_arg`)... plus what you said above about Clang's internal implementation; so that's a good reason to stick with putting the attribute on the template.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311