[PATCH] D137909: [Support] Allow complex names for annotation points and ranges via ${}

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

TL;DR: I think the `${}` is a nice quality-of-life improvement as-is, but attaching complex metadata needs something more. And we should spend our syntax-complexity-budget on the latter. What about making payload a separate attribute of points, with syntax `$name(payload)^`, which collapses in simple cases to `$name^` or `$(payload)^` or just `^`?

---

In D137909#3924611 <https://reviews.llvm.org/D137909#3924611>, @tom-anders wrote:

> This clangd test <https://searchfox.org/llvm/source/clang-tools-extra/clangd/unittests/XRefsTests.cpp#1933> (and some others in this file) use the names of annotation for encoding the expected `Attribute` field of the `Reference`struct.
> For testing https://github.com/clangd/clangd/issues/177 (https://reviews.llvm.org/D137894) I wanted to extend those tests instead of writing new one, so the idea was to encode both the expected `Attribute` and the expected `Context` inside the annotation name (e.g. via comma-separated list).

Right, I think this is a slightly bigger can of worms...

Annotations was designed to support lookup of points/ranges with known names and the API works pretty well for this purpose without being complicated.
The tests you link work OK because we're looking up a fixed set of names (unnamed, def, decl). Using names for metadata beyond basic categories doesn't work well because you need the name to look them up. (We sometimes hack around this, at the cost of test clarity/simplicity).

Metadata comes up enough that it might be worth accommodating. Simply providing iterators over (name,point) pairs would make this possible but I don't think overloading names in this way is a great design:

- makes it less obvious what a name is
- makes llvm::Annotations API harder to understand
- means names + metadata can't be used together

I think it would be better to make metadata orthogonal to names. Silly strawman syntax:

  ^  // just a point
  @foo^ // unnamed (i.e. def+decl), container is foo
  $def^ // definition, no container
  $decl at foo^ // definition, contained by foo

then add functions to Annotations like `ArrayRef<pair<unsigned, StringRef>> pointsWithPayload(StringRef Name = "")`

Of course good syntax is both important and hard: `@` isn't really available as a sigil (ObjC), and brackets are nicer in these complicated chains than prefix sigils too... and we need to be backwards compatible.

What do you think about `$name(payload)^` for the general case, with `$(payload)^`, `$name`, and `^` as abbreviated forms? I don't think the lack of brackets on **names** hurts a lot if they stay simple, and if they're "just labels" that don't have to carry arbitrary payloads then they can be simple.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137909