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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:49
+    Result = New;
+  else if (Result != New)
+    Result = SelectionTree::Partial;
----------------
I might be missing something, I don't understand why we set Partial if `Result != New`.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:446
+    // Consider the macro expansion FLAG(x) -> int x = 0;
+    // Neither S.getBegin() nor S.getEnd() are arg expansions, but x is.
+    // The selection FLAG([[x]]) must partially select the VarDecl.
----------------
what's S for this case?


================
Comment at: clang-tools-extra/clangd/Selection.cpp:464
+      // represent one AST construct, but a macro invocation can represent many.
+      if (FID == SelFile) {
+        // Tokens written directly in the main file.
----------------
nit: maybe use [`early-exist`](https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code) here?


================
Comment at: clang-tools-extra/clangd/Selection.cpp:496
+          // Check if the macro name is selected, don't claim it exclusively.
+          auto Expansion = SM.getDecomposedExpansionLoc(S.getBegin());
+          if (Expansion.first == SelFile)
----------------
not sure what's the rational behavior, but I think for the following case, we just have the TUDecl in the selection tree, maybe use the whole macro range?

```
#define M() 123

int d = M(^); // now we only have the TUDecl in the selection tree.
```


================
Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:139
+          R"cpp(
+            int x(int);
+            #define M(foo) x(foo)
----------------
could we have a  testcase to cover the "tokens expanded from another #include file" code path?



================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:125
+    return {};
+  const Token *Begin =
+      llvm::partition_point(expandedTokens(), [&](const syntax::Token &T) {
----------------
nit: for code readability, I'd use `llvm::ArrayRef<syntax::Token>::iterator` type here.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:126
+  const Token *Begin =
+      llvm::partition_point(expandedTokens(), [&](const syntax::Token &T) {
+        return SourceMgr->isBeforeInTranslationUnit(T.location(), R.getBegin());
----------------
I think the parition_point requires the `ExpandedTokens` is partitioned, but I didn't found any documentation about this guarantee in the code, would be nice to have this in the comment (probably around the `ExpandedTokens`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70512