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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
