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

Mon Nov 16 17:23:52 PST 2020

rsmith marked an inline comment as done.
rsmith added a comment.

In D91311#2398144 <https://reviews.llvm.org/D91311#2398144>, @ldionne wrote:

> What the attribute achieves is great, however I must say I'm really with Arthur's original comment regarding the ergonomics of it.

I do agree, the ergonomics aren't good. Due to the way that instantiations of attributes on templates work, we end up needing to see the attribute no later than on the definition of the template, which forces the template to be declared at least twice, and similarly the typedef needs to be named at least twice (once to declare it, and once to annotate it on the template). It would certainly be more convenient to attach the attribute to the typedef.

> IMO, it makes a lot more sense to permit the typedef author to pick how their typedef is going to be "named" by the compiler.

Well, this attribute doesn't affect how the typedef is named -- if we know the type was named via a typedef, we'll use that typedef-name to describe the type regardless of this attribute. For example, if you have `using Name = std::basic_string<char>;` and then use the type `Name`, we're going to call the type `Name` everywhere we have tracked enough information to know that you used a typedef. Rather, the attribute affects how a template specialization is named in the case where we can't track it back to a typedef or other type sugar -- for example, if in a use of `Name` we lose the association back to the typedef `Name` and only know that it's `std::basic_string<char>`, we'll call it `std::string` instead. So this really changes a property of the template (or more accurately, a specialization of the template), not a property of the typedef.

I think allowing the attribute on a typedef would be misleading: people would, quite reasonably, expect that they can specify the attribute on an arbitrary typedef and that typedef name would be preferred when printing that type in general. We could in principle support such a generalized attribute on typedefs, but I don't think that would be a good idea. Such an attribute would be very complex to support (primarily due to lazy loading of module files), and I'd expect it to be heavily abused, with people "renaming" built-in types, pointer types, etc. in ways that are unhelpful to users of their code.

> If they pick something crazy or misleading, it's really their problem.

I don't entirely agree with this part -- such misuse would be a problem for anyone who uses the header containing the typedef and now sees type names in diagnostics that have nothing to do with the code in question. Nonetheless, misuse is the problem of the people misusing the attribute and their users; it's not necessarily our problem, and we don't necessarily need to design an attribute that can't be misused.

> 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).

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.

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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91311