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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Fantastic, thanks!



================
Comment at: clang-tools-extra/clangd/unittests/Annotations.h:29
 
-  Position point(llvm::StringRef Name = "") const;
+  clangd::Position point(llvm::StringRef Name = "") const;
+  std::pair<clangd::Position, llvm::StringRef>
----------------
Nit: no need for clangd:: qualifiers here or in cpp file, apart from on Range (to distinguish from llvm:: Annotations::Range)


================
Comment at: llvm/include/llvm/Testing/Support/Annotations.h:28
 ///       $definition^class Foo{};          // points can be named: "definition"
+///       $definition(foo)^class Foo{};     // and/or contain a payload: "foo"
 ///       $fail[[static_assert(false, "")]] // ranges can be named too: "fail"
----------------
Nit: I'd probably rather show payload both with and without name here, and then not bother repeating different combinations for range:

```
$definition^ // points can be named: "definition"
$(foo)^ // or have a payload: "foo"
$definition(foo)^ // or both
$fail[[...]] // ranges can have names/payloads too
```


================
Comment at: llvm/include/llvm/Testing/Support/Annotations.h:28
 ///       $definition^class Foo{};          // points can be named: "definition"
+///       $definition(foo)^class Foo{};     // and/or contain a payload: "foo"
 ///       $fail[[static_assert(false, "")]] // ranges can be named too: "fail"
----------------
sammccall wrote:
> Nit: I'd probably rather show payload both with and without name here, and then not bother repeating different combinations for range:
> 
> ```
> $definition^ // points can be named: "definition"
> $(foo)^ // or have a payload: "foo"
> $definition(foo)^ // or both
> $fail[[...]] // ranges can have names/payloads too
> ```
Nit: examples here are realistic, so avoid "foo". Maybe "complete"?


================
Comment at: llvm/include/llvm/Testing/Support/Annotations.h:78
   size_t point(llvm::StringRef Name = "") const;
+  /// Returns the position of the point marked by ^ (or $name(payload)^) in the
+  /// text containing its payload (or llvm::None if there is no payload).
----------------
Nit: I think it's more valuable to keep the one-sentence summary to one line (as above) than to repeat (some of) the syntax options again here, since this is clearly a variant of the method above.
Also the llvm::Optional part is obsolete.

Maybe `Return the position of the named marked point, and its payload if any.`?


================
Comment at: llvm/include/llvm/Testing/Support/Annotations.h:93
   /// sorted.
-  const llvm::StringMap<llvm::SmallVector<size_t, 1>> &all_points() const;
+  llvm::StringMap<llvm::SmallVector<size_t, 1>> all_points() const;
 
----------------
Can you add a FIXME here and on `all_points()` to remove these functions in favor of exposing `All` directly?

(It's great that this isn't done in this patch though: that API break will cause some out-of-tree headaches I'm happy to deal with myself)


================
Comment at: llvm/include/llvm/Testing/Support/Annotations.h:120
+    size_t Begin;
+    size_t End;
+    llvm::StringRef Name;
----------------
Document the sentinel value, or maybe better

```
size_t End = -1;
bool isPoint() const { return End == size_t(-1); }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137909