[PATCH] D59814: [Testing] Move clangd::Annotations to llvm testing support

[Sam McCall via Phabricator via llvm-commits]

sammccall added a comment.

This probably needs tests if we're lifting it into `llvm`. Sorry there aren't any today :-(

BTW the other similar lib in clang I'm aware of is https://reviews.llvm.org/diffusion/L/browse/cfe/trunk/tools/clang-refactor/TestSupport.h
It makes different tradeoffs.

We talked offline about the design choices (particularly the markings used) that make annotations terse but limit the applicability, e.g. code containing `^` cannot be annotated. (I'd say the limitations have been OK for clangd). Possible changes:

- add an escaping mechanism so that currently impossible literal code can be encoded. e.g. `^^` is a literal `^`, or `$^` is a literal `^` or similar. I'd be fine with almost any option here.
- allow users of the library to customize the markings used (e.g. pass in an `Annotations::Sigils` struct, and provide defaults and possibly an alternate set)
- change the markings to make them more verbose, e.g. `$^` or `#^#` instead of `^`. Verbosity is clearly a question of taste here, I'm not particularly in favor of such a change. Choice of markings is a complicated and constrained problem.
- change the markings to make them less ambiguous without being more verbose, e.g. `$` instead of `^` and `|[foo]|` instead of `[[foo]]`. I'm more amenable to this, though choice of markings remains important and hard.
- add a `FIXME` promising to do any of these things in the future. This option is certainly fine from clangd's perspective. This probably forces future changes to be backwards compatible or have broad consensus.



================
Comment at: clang-tools-extra/unittests/clangd/Annotations.h:8
 //===----------------------------------------------------------------------===//
-//
-// Annotations lets you mark points and ranges inside source code, for tests:
-//
-//    Annotations Example(R"cpp(
-//       int complete() { x.pri^ }          // ^ indicates a point
-//       void err() { [["hello" == 42]]; }  // [[this is a range]]
-//       $definition^class Foo{};           // points can be named: "definition"
-//       $fail[[static_assert(false, "")]]  // ranges can be named too: "fail"
-//    )cpp");
-//
-//    StringRef Code = Example.code();              // annotations stripped.
-//    std::vector<Position> PP = Example.points();  // all unnamed points
-//    Position P = Example.point();                 // there must be exactly one
-//    Range R = Example.range("fail");              // find named ranges
-//
-// Points/ranges are coordinates into `code()` which is stripped of annotations.
-//
-// Ranges may be nested (and points can be inside ranges), but there's no way
-// to define general overlapping ranges.
-//
+// A clangd-specific version of llvm/Testing/Support/Annotations.h, replaces
+// offsets and offset-based ranges with types from the LSP protocol.
----------------
The choice to shadow the base methods is interesting. I guess we can always use llvm::Annotations if we want byte offsets (I know there are some), not sure if we ever want a mix.
Anyway this is nice and clean, and minimizes the diff. We can change later if we care.


================
Comment at: llvm/include/llvm/Testing/Support/Annotations.h:42
+/// represents a half-open range.
+struct Range {
+  size_t Begin = 0;
----------------
ilya-biryukov wrote:
> This name is probably too generic, happy to change the name or implement this in an alternative manner.
I'd probably suggest making this `Annotations::Range`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59814