[PATCH] D64562: [clangd] Fixed toHalfOpenFileRange

[Sam McCall via Phabricator via cfe-commits]

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