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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
