[PATCH] D67358: [clangd] Implement semantic selections.

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 3 inline comments as done.
sammccall added a comment.

Nice! Particularly: great tests.

Only big thing is toHalfOpenFileRange should get you substantially better macro handling.



================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:18
+namespace {
+void addIfUnique(const Range &R, SemanticSelectionResult *Result) {
+  if (Result->Ranges.empty() || Result->Ranges.back() != R) {
----------------
nit: "unique" suggests this function checks against all the ranges in *Result (which it doesn't, and indeed doesn't need to)
I'd suggest renaming `addIfDistinct` and adding a comment that only consecutive ranges should be able to coincide


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:33
+  if (!Offset) {
+    llvm::errs() << "Unable to convert postion to offset";
+    return {};
----------------
we use log() or elog() for this, make sure you include Offset.takeError() for the original message

Alternatively, you might consider returning Expected<SemanticSelectionResult> so the error gets propagated to the client (as this really is a client error)


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:39
+  SelectionTree ST(AST.getASTContext(), AST.getTokens(), *Offset);
+  for (const auto *Node = ST.commonAncestor(); Node != nullptr;
+       Node = Node->Parent) {
----------------
I think you probably want to bail out once you hit TranslationUnitDecl.

And maybe at namespace decls? Though I have often in my editor wondered "what's the enclosing namespace"...


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:45
+    }
+    SourceLocation BeginLoc = SR.getBegin();
+    SourceLocation EndLoc =
----------------
Ah, this will work if the start and the end of the AST node are written directly in the file, and no macros are involved. But try `toHalfOpenFileRange` in `SourceCode.h`, which should work in a lot more cases.

In particular, this should work:
```
#define INC(X) X + 1
int a, b;
int c = INC(a * b);
            ^
            ~
            ~~~~~
        ~~~~~~~~~~
~~~~~~~~~~~~~~~~~~
```


================
Comment at: clang-tools-extra/clangd/SemanticSelection.h:21
+
+struct SemanticSelectionResult {
+  // The list of interesting selections around the cursor. 
----------------
I'm not sure this struct pulls its weight, I'd suggest just returning vector<Range> here.
This isn't a stable API, we can change it if we need to add things.
Or is there something you have in mind?

Often we mirror the LSP here, but LSP is weird enough here (a linked list, seriously?) that I don't particularly think we should.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.h:27
+// Returns the list of all interesting ranges around the Position \p Pos. 
+// Constructs the 
+// Any range in the result strictly contains all the previous ranges in the result.
----------------
incomplete sentence. It'd be nice to spell out on this line what an "interesting" range is (just one that corresponds to an AST node, right?)


================
Comment at: clang-tools-extra/clangd/SemanticSelection.h:28
+// Constructs the 
+// Any range in the result strictly contains all the previous ranges in the result.
+SemanticSelectionResult getSemanticRanges(ParsedAST &AST, Position Pos);
----------------
somewhere you should mention that front() is the inner most range, and back() the outermost.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:53
+       )cpp",
+      R"cpp( // Cursor inside a macro.
+        #define HASH(x) ((x) % 10)
----------------
this is the case where `toHalfOpenFileName` should help


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:59
+       )cpp",
+      R"cpp( // Cursor on a macro.
+        #define HASH(x) ((x) % 10)
----------------
interesting, this *might* be worth special handling, to select the whole macro expansion. Wonder if people will complain. Let's leave it for now for simplicity


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:82
+      )cpp",
+      // FIXME: No node found associated to the position.
+      R"cpp( // Cursor at end of VarDecl.
----------------
If you're interested, I think the fix here is in `pointBounds()` in selection.cpp.
Since selection-tree now treats semicolons as if they don't exist, we should probably handle them the same way we do whitespace, and look left instead of right.

(definitely a different patch though)


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:88
+      )cpp",
+      // FIXME: No node found associated to the position.
+      R"cpp( // Cursor at end of VarDecl.
----------------
oh, this is interesting. it's a side-effect of how selectiontree now ignores whitespace (and semicolons, and comments).

We convert a cursor into a size-one selection, and that used to always select something, but no longer does.
This is unfortunate but hopefully pretty rare. If it's common to e.g. want to select a functiondecl while on a blank line inside it, we may want to revisit this.


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:115
+            [[void func() [[{
+              // int x = nsa::nsb::ccc();
+              [[[[int x = [[[[nsa::nsb::cc^c]]()]]]];]]
----------------
why commented out?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358