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

[Tom Praschan via Phabricator via llvm-commits]

tom-anders added a comment.

In D137909#3930043 <https://reviews.llvm.org/D137909#3930043>, @sammccall wrote:

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

Thanks for the detailed feedback, I think this looks perfect! I adapted the code and docs accordingly (I added some comments to the parts I'm not sure about)

In D137909#3930047 <https://reviews.llvm.org/D137909#3930047>, @sammccall wrote:

>> D137894 <https://reviews.llvm.org/D137894>
>
> this looks cool! I left a couple of comments, mentioning here because phab won't notify for comments on drafts.

Thanks! I replied to one of them :)



================
Comment at: clang-tools-extra/clangd/unittests/Annotations.h:30
 
+  struct Position : clangd::Position {
+    llvm::Optional<llvm::StringRef> Payload;
----------------
I thought about just using `std::pair<clangd::Position, llvm::Optional<llvm::StringRef>> instead, but that is pretty verbose and unreadable here IMO. Another nice thing about subclassing here is that we can keep the API stable and don't have to introduce  an additional `pointWithPayload()` getter like in the base class.


================
Comment at: llvm/include/llvm/Testing/Support/Annotations.h:61
     size_t End = 0;
+    llvm::Optional<llvm::StringRef> Payload;
 
----------------
One could argue that this should just be `llvm::StringRef`. Right now, `$name^` results in `llvm::None` for the payload, while `$name()^` will yield an empty string.

Do you think they should just both be empty strings instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137909