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

Tue Nov 17 05:54:29 PST 2020

aaron.ballman added a comment.

In D91311#2398526 <https://reviews.llvm.org/D91311#2398526>, @rsmith wrote:

> In D91311#2398144 <https://reviews.llvm.org/D91311#2398144>, @ldionne wrote:
>
>> I think that the fact we need to re declare everything shows how the ergonomics would be better if we could just add the attribute to the typedef. See for example how we're re-declaring a bunch of stuff in `<iosfwd>`. How bad of an idea is it to try putting the attribute on typedefs instead, and how strongly are you opposed to it? Because from a naive user perspective, having to redeclare the class with a closed set of preferred names feels awkward (IMO, of course).

FWIW, I'd be pretty strongly opposed to putting the attribute on the typedef instead, for all the reasons Richard points out. It's the wrong subject for the attribute given the semantics of how the attribute behaves and I'd liken it somewhat to writing an attribute on a function declaration when the semantics of the attribute impact a parameter of the function instead. While the function and its parameter are certainly related (more strongly than the template and the typedef in this case), it's just the wrong place to write the attribute.

> How much should we concern ourselves with ergonomics in this instance? Most of the intended uses of this attribute (at least by how often they'll affect diagnostics) are included in this patch, and are in code that we don't expect many people to look at most of the time, that is maintained by domain experts, and that is generally optimized for other properties than readability. However, this is certainly intended to be used by third-party library authors; otherwise we could just add a hard-coded table to Clang itself. So I think ergonomics do matter a little, but that other factors are probably more important.

I think the ergonomics matter because it's a documented attribute that people will use outside of the STL, but I don't imagine this being an attribute that gets written frequently. I'm fine if the usage is a bit verbose in this case.

> We could allow the attribute on typedefs, and internally reverse the sense of it and attach it to the template specialization instead, but that would be incredibly messy -- in order to maintain AST invariants and source fidelity, we'd need to synthesize a new declaration of the template specialization to attach the attribute to, or something similar. We'd still need the attribute to appear before the template definition, though -- unless we rework how attribute instantiation is done -- so that's not really a general solution to the ergonomics issue. Or we could store a side-table, which would also bring with it a lot of complexity, particularly when we come to lazily load such information from module files. In my view, the added implementation complexity from attaching the attribute to a typedef outweighs the ergonomic benefit.
>
> There's another design approach we could follow, that would keep the attribute on the template but avoid the awkwardness of requiring the typedef to appear first: we could completely divorce this feature from typedefs. Instead, we could say the attribute is of the form `preferred_name(type, "name")`, where `type` is a specialization of the type to which the attribute is attached, and `"name"` is a name used in diagnostics when referring to that type, which is printed as if it's a member of the enclosing namespace (but there'd be no enforcement that such a member actually exists and declares the corresponding type). What do you think?

I'm not opposed to that approach, but I don't think the design is as nice as what's originally proposed in terms of behavior. With the current approach, the name that gets used in diagnostics is one that shows up in the code as an actual declaration of something and I think that's a useful property to enforce. I don't like a design where the user can write an arbitrary string literal that has no enforcement that it actually names a type in the user's program. While doing such a thing is up to the author of the attribute and so it's sort of a "well then don't do that if it's nonsense" situation, it's also not hard to get accidental typos that slip through code reviews but would be caught by using the actual type system itself (which provides extra value like diagnostics about the mistake or possible typo correction).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311