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

[UTKARSH SAXENA via Phabricator via cfe-commits]

usaxena95 added inline comments.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:33
+  if (!Offset) {
+    llvm::errs() << "Unable to convert postion to offset";
+    return {};
----------------
sammccall wrote:
> 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)
Returning Expected<vector<Ranges>> now to propagate the 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) {
----------------
sammccall wrote:
> 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"...
Bailing out once we hit the TUDecl. I am not sure about exiting on namespace decl. I am keeping this for now. We can remove this later if the need be.


================
Comment at: clang-tools-extra/clangd/SemanticSelection.cpp:45
+    }
+    SourceLocation BeginLoc = SR.getBegin();
+    SourceLocation EndLoc =
----------------
sammccall wrote:
> 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);
>             ^
>             ~
>             ~~~~~
>         ~~~~~~~~~~
> ~~~~~~~~~~~~~~~~~~
> ```
Interesting. This works fine in such cases. Thanks.
Though it works strangely with these ugly macros. I guess this is because it tries to follow the macro expansion.
```
#define MUL ) * (
int var = (4 + 15 MUL 6 + 10);
                ^
               ~~
           ~~~~~~
          ~~~~~~~~~~~
```


================
Comment at: clang-tools-extra/clangd/unittests/SemanticSelectionTests.cpp:115
+            [[void func() [[{
+              // int x = nsa::nsb::ccc();
+              [[[[int x = [[[[nsa::nsb::cc^c]]()]]]];]]
----------------
sammccall wrote:
> why commented out?
Since the statement contains many [[ and ]], I have added the actual statement in comments to make it more readable. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67358