[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#3933730 <https://reviews.llvm.org/D137909#3933730>, @sammccall wrote:

> Thanks! sorry about continuing to drift this patch, but this looks like a great improvement.

No problem! I wasn't quite happy yet with the previous version of this patch either, really appreciate the continuous feedback!



================
Comment at: llvm/include/llvm/Testing/Support/Annotations.h:58
   /// represents a half-open range.
   struct Range {
     size_t Begin = 0;
----------------
sammccall wrote:
> this patch takes too many different approaches to attaching the payloads to the points:
>  - adding it to an existing struct that is already returned
>  - defining a new wrapping struct for the andPayload version
>  - defining a new inheriting struct for the andPayload version
> 
> I think we should keep it simple, and use `pair<T, StringRef>` for all the `andPayload` versions. This isn't optimal in isolation, but it keeps things coherent, avoiding:
>  - confusion around clever type-system features (inheritance, implicit conversions, smart-pointer like types, templates)
>  - introducing new structs whose names we have to remember
>  - inconsistency between how payloads are accessed between ranges/points/clangd/llvm versions
> 
> In many cases structured bindings make using the `pair` versions less cumbersome than they would have been in the past.
Yeah structured bindings help a lot here. Also, gtest has a `Pair` matcher which is really nice to use for this stuff


================
Comment at: llvm/include/llvm/Testing/Support/Annotations.h:108
+  const llvm::StringMap<llvm::SmallVector<Point, 1>> &
+  all_points_and_payloads() const;
 
----------------
sammccall wrote:
> I hadn't seen these context on these `all_` functions, I reached out to the authors to find out about them (they're used out-of-tree).
> 
> Basically the use case is to use llvm::Annotations to do the parsing, but dump out the data into some external test helper which lookups/assertions will run on.
> So instead of adding extra variants, we just need to expose the parse results in some convenient way.
Ok, got rid of `all_points_and_payloads()` and kept `all_points()` as-is for now


================
Comment at: llvm/lib/Testing/Support/Annotations.cpp:30
   };
   llvm::Optional<llvm::StringRef> Name;
+  llvm::Optional<llvm::StringRef> Payload;
----------------
sammccall wrote:
> (If using the `Annotation` struct I mentioned before, an instance of it could cover Name+Payload and it could also be the elements of OpenRanges)
Felt a bit less readable to me, so I kept the two optionals for name and payload


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137909