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