[PATCH] D70512: [clangd] Rethink how SelectionTree deals with macros and #includes.

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added a comment.
This revision is now accepted and ready to land.

Thanks, the patch looks good! please also update the patch description.



================
Comment at: clang-tools-extra/clangd/Selection.cpp:63
+  // Claims whichever expanded tokens in SourceRange are not yet claimed.
+  // Writes contiguous claimed subranges to Result, and returns true if any.
+  llvm::SmallVector<llvm::ArrayRef<T>, 4> erase(llvm::ArrayRef<T> Claim) {
----------------
nit: this comment seems not reflect to the code now.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:126
+
+// Nodes start start with NoTokens, and then use this function to aggregate
+// the selectedness as more tokens are found.
----------------
nit: remove the redundant start.


================
Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:468
+  // Unfortunately, this makes the common ancestor the CallExpr...
+  // FIXME: hack around this by picking one?
+  EXPECT_EQ("CallExpr", T.commonAncestor()->kind());
----------------
I think cases like below are broken:

```
    #define greater(x, y) x > y? x : y
    #define abs(x) x > 0 ? x : -x
```

 Selecting the first element for a macro arg seems good to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70512