Page MenuHomePhabricator

[Testing] Move clangd::Annotations to llvm testing support
ClosedPublic

Authored by ilya-biryukov on Mar 26 2019, 4:17 AM.

Details

Summary

Annotations allow writing nice-looking unit test code when one needs
access to locations from the source code, e.g. running code completion
at particular offsets in a file. See comments in Annotations.cpp for
more details on the API.

Also got rid of a duplicate annotations parsing code in clang's code
complete tests.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Mar 26 2019, 4:17 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 26 2019, 4:17 AM
ilya-biryukov marked an inline comment as done.Mar 26 2019, 4:21 AM

Seeking feedback for this change, mostly interested if this a good place for test helpers like this?
I'm planning to author more tests in clangTooling using this soon, so moving it somewhere clang-specific is an option too.

llvm/include/llvm/Testing/Support/Annotations.h
42 ↗(On Diff #192258)

This name is probably too generic, happy to change the name or implement this in an alternative manner.

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.
clang-tools-extra/unittests/clangd/Annotations.h
8 ↗(On Diff #192258)

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.

llvm/include/llvm/Testing/Support/Annotations.h
42 ↗(On Diff #192258)

I'd probably suggest making this Annotations::Range

gribozavr accepted this revision.Mar 28 2019, 9:53 AM
gribozavr added inline comments.
llvm/include/llvm/Testing/Support/Annotations.h
9 ↗(On Diff #192258)

Move into a doc comment on the class Annonations?

49 ↗(On Diff #192258)

Three slashes for doc comments.

This revision is now accepted and ready to land.Mar 28 2019, 9:53 AM
ilya-biryukov marked 4 inline comments as done.
  • Move Range into the body of Annotations
  • Use triple-slash comments
  • Added a FIXME that we might want to change the syntax
  • Move the doc comment to the class

I've added a FIXME to the class.
Also want to add some basic tests before landing this.

clang-tools-extra/unittests/clangd/Annotations.h
8 ↗(On Diff #192258)

Exactly my thoughts. I don't expect us to have places that need both the offsets and the positions.

  • Add simple tests
  • Add equality and stream output operators for Range
  • Added a death test for error conditions
sammccall accepted this revision.Apr 25 2019, 3:05 AM
sammccall added inline comments.
llvm/lib/Testing/Support/Annotations.cpp
94 ↗(On Diff #196592)

You could consider including the stringref in the Range struct just to make it print more nicely.
Not sure of all the implications of this, up to you.

llvm/unittests/Support/AnnotationsTest.cpp
87 ↗(On Diff #196592)

you could consider some #ifndef NDEBUG EXPECT_DEATH tests for point() with no/multiple points etc. Up to you.

This revision was automatically updated to reflect the committed changes.