[PATCH] D64562: [clangd] Fixed toHalfOpenFileRange

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/SourceCode.cpp:261
+  if (!Lexer::getRawToken(Loc, TheTok, SM, LangOpts)) {
+    if (TheTok.is(tok::greatergreater))
+      return 1;
----------------
sammccall wrote:
> `>>=` too? That's legal with variable templates.
> 
> (If you're deliberately choosing different behavior that definitely deserves a comment)
nit: please don't just mark comments "done" if there's reason not to do them - a brief explanation in the review and/or the code is useful to keep track.

For the record, it turns out that it seems C++ doesn't actually allow `>>=` to be ambiguous - the user has to put a space before = if it's not a shift.


================
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) {
----------------
SureYeaah wrote:
> sammccall wrote:
> > 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.
> e.g. Nested template instantiation (TemplateSpecializationTypeLoc)
That's a type of decl, not a type of range.

This function inputs and outputs a range, so the type of decl used isn't relevant to its correctness.
Is there a particular type of range (e.g. certain macro expansion structure and pair of points) that you want to test but can't construct a decl to cover?


================
Comment at: clang-tools-extra/clangd/unittests/SourceCodeTests.cpp:421
+      $c[[FOO(b, c)]]; 
+      $d[[FOO(BAR(BAR(b)), d)]];
+    }
----------------
SureYeaah wrote:
> sammccall wrote:
> > 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)
> In this case, the FileRange would be
> 
> ```
> ECHO(ECHO($e[[int) ECHO(e]]));
> ```
> 
> So the code works correctly. But it's not how we want it to behave right?
That looks OK to me. It's surprising, but I think defensible.

The most likely source of complaints is I guess if we use selection tree to implement structured selection expansion. Anyway, whatever the behavior is, thanks for adding the test :-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64562