sammccall added a comment.
Thanks for fixing this, this stuff is such a tangled web.
Logic seems great, so just style/test nits.
================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:261
+ if (!Lexer::getRawToken(Loc, TheTok, SM, LangOpts)) {
+ if (TheTok.is(tok::greatergreater))
+ return 1;
----------------
`>>=` too? That's legal with variable templates.
(If you're deliberately choosing different behavior that definitely deserves a
comment)
================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:303
+
+// returns the tokenFileRange for a given Location as a Token Range
+static SourceRange getTokenFileRange(SourceLocation Loc,
----------------
Explain why we're implementing this from scratch?
Maybe something like
```
This is similar to getFileLocation in SourceManager, which chooses
getExpansionRange or getSpellingLocation (for macro arguments).
However:
- we need full range information, which getFileLocation discards
- (there's something else wrong with getExpansionRange, right?)
- we want to split `>>` tokens
```
================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:338
+ SourceRange Result = unionTokenRange(R1, R2, SM, LangOpts);
+ unsigned TokLen = getTokenLengthAtLoc(Result.getEnd(), SM, LangOpts);
+ Result.setEnd(Result.getEnd().getLocWithOffset(TokLen));
----------------
maybe a comment "convert from closed token range to half-open range"
================
Comment at: clang-tools-extra/clangd/SourceCode.h:69
+/// Turn a SourceLocation into a pair of positions
+Range sourceRangeToRange(const SourceManager &SM, SourceRange SrcRange);
+
----------------
This isn't well-defined without more information, because a SourceRange may be
token/char range etc.
(And in fact this appears to treat it as a half-open range, which isn't the
most common type).
Because this is ambiguous, fairly rare, and simple, can we just inline it into
the test where it's used instead?
================
Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:410
+// Test for functions toHalfOpenFileRange and getHalfOpenFileRange
+// FIXME: Need better testing support to be able to check more than just Decls.
+TEST(SourceCodeTests, HalfOpenFileRange) {
----------------
this is a function on ranges, so only using decls isn't a limitation per se.
Is there a type of range you're unable to test because you can't construct it
as the source range of a decl? If so, please say which. If not I think we
should just drop this comment.
================
Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:412
+TEST(SourceCodeTests, HalfOpenFileRange) {
+ Annotations Test(R"cpp(
+ #define FOO(X, Y) int Y = ++X
----------------
Can you add an explanation of what's in the test?
e.g. "each marked range should be the file range of the decl with the same name"
================
Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:421
+ $c[[FOO(b, c)]];
+ $d[[FOO(BAR(BAR(b)), d)]];
+ }
----------------
some tests where the expansion range is a macro arg? e.g.
```
#define ECHO(X) X
ECHO($e[[ECHO(int) ECHO(e)]])
```
(if I'm understanding right)
================
Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:432
+ auto FileRange = toHalfOpenFileRange(SM, LangOpts, Decl.getSourceRange());
+ ASSERT_NE(FileRange, llvm::None);
+ Range HalfOpenRange = sourceRangeToRange(SM, *FileRange);
----------------
I like the "factored out" test, but one trap is that if assertions fail then
you can't tell which case it was.
You can use SCOPED_TRACE(Name) at the top of this block to make sure we print
the range name.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64562/new/
https://reviews.llvm.org/D64562
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
