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