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

[Sam McCall via Phabricator via cfe-commits]

sammccall planned changes to this revision.
sammccall added a comment.

No, this patch is busted, and the tests were too simple go catch it.

The issue is that with no exclusivity check, given `{{{ MACRO }}}` all the enclosing blocks get to count themselves selected.

We need an exclusivity check on the expanded token stream first. This will yield a sequence of slices of tokens not yet claimed. Then each slice gets partitioned by FileID, each partition gets mapped to spelled tokens and checked as in this patch.

I suspect the exclusivity/claiming at the spelled token level is no longer needed.



================
Comment at: clang-tools-extra/clangd/Selection.cpp:49
+    Result = New;
+  else if (Result != New)
+    Result = SelectionTree::Partial;
----------------
hokein wrote:
> I might be missing something, I don't understand why we set Partial if `Result != New`.
Added a brief comment and an assertion.
Intuitive explanation: Consider `Complete` = black, `Unselected` = white, `Partial` = grey.
White + White -> white, Black + Black -> black, every other mixture is grey.

(But honestly I wrote up the state transition table first and then extracted the logic by staring at it)


================
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.
----------------
hokein wrote:
> what's S for this case?
S is the function parameter, the implication is that it's the range of the vardecl in the example. Expanded the comment a bit.


================
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)
----------------
hokein wrote:
> 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.
> ```
This is definitely better, but isn't trivial to do, and isn't a very important case.
Added a FIXME rather than clutter/delay this patch with it.


================
Comment at: clang/lib/Tooling/Syntax/Tokens.cpp:125
+    return {};
+  const Token *Begin =
+      llvm::partition_point(expandedTokens(), [&](const syntax::Token &T) {
----------------
hokein wrote:
> nit: for code readability, I'd use `llvm::ArrayRef<syntax::Token>::iterator` type here.
I think that hurts readability on two counts:
 - it obscures the actual type: a pointer is concrete and familiar, so it's easier to realize that e.g. `>` is well-defined here
 - it makes it harder to understand `return {Begin, End}` which relies on the fact that the actual type here is Token*


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70512