nridge updated this revision to Diff 238094.
nridge added a comment.
Revise the approach to treat non-first expansions as not-selected
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72041/new/
https://reviews.llvm.org/D72041
Files:
clang-tools-extra/clangd/Selection.cpp
clang-tools-extra/clangd/Selection.h
clang-tools-extra/clangd/unittests/SelectionTests.cpp
clang-tools-extra/clangd/unittests/XRefsTests.cpp
Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -334,7 +334,18 @@
#define ADDRESSOF(X) &X;
int *j = ADDRESSOF(^i);
)cpp",
-
+ R"cpp(// Macro argument appearing multiple times in expansion
+ #define VALIDATE_TYPE(x) (void)x;
+ #define ASSERT(expr) \
+ do { \
+ VALIDATE_TYPE(expr); \
+ if (!expr); \
+ } while (false)
+ bool [[waldo]]() { return true; }
+ void foo() {
+ ASSERT(wa^ldo());
+ }
+ )cpp",
R"cpp(// Symbol concatenated inside macro (not supported)
int *pi;
#define POINTER(X) p ## X;
Index: clang-tools-extra/clangd/unittests/SelectionTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/SelectionTests.cpp
+++ clang-tools-extra/clangd/unittests/SelectionTests.cpp
@@ -368,7 +368,7 @@
// Regression test: this used to match the injected X, not the outer X.
TEST(SelectionTest, InjectedClassName) {
- const char* Code = "struct ^X { int x; };";
+ const char *Code = "struct ^X { int x; };";
auto AST = TestTU::withCode(Annotations(Code).code()).build();
auto T = makeSelectionTree(Code, AST);
ASSERT_EQ("CXXRecordDecl", nodeKind(T.commonAncestor())) << T;
@@ -461,7 +461,8 @@
}
TEST(SelectionTest, MacroArgExpansion) {
- // If a macro arg is expanded several times, we consider them all selected.
+ // If a macro arg is expanded several times, we only consider the first one
+ // selected.
const char *Case = R"cpp(
int mul(int, int);
#define SQUARE(X) mul(X, X);
@@ -470,15 +471,8 @@
Annotations Test(Case);
auto AST = TestTU::withCode(Test.code()).build();
auto T = makeSelectionTree(Case, AST);
- // Unfortunately, this makes the common ancestor the CallExpr...
- // FIXME: hack around this by picking one?
- EXPECT_EQ("CallExpr", T.commonAncestor()->kind());
- EXPECT_FALSE(T.commonAncestor()->Selected);
- EXPECT_EQ(2u, T.commonAncestor()->Children.size());
- for (const auto* N : T.commonAncestor()->Children) {
- EXPECT_EQ("IntegerLiteral", N->kind());
- EXPECT_TRUE(N->Selected);
- }
+ EXPECT_EQ("IntegerLiteral", T.commonAncestor()->kind());
+ EXPECT_TRUE(T.commonAncestor()->Selected);
// Verify that the common assert() macro doesn't suffer from this.
// (This is because we don't associate the stringified token with the arg).
@@ -495,7 +489,7 @@
}
TEST(SelectionTest, Implicit) {
- const char* Test = R"cpp(
+ const char *Test = R"cpp(
struct S { S(const char*); };
int f(S);
int x = f("^");
Index: clang-tools-extra/clangd/Selection.h
===================================================================
--- clang-tools-extra/clangd/Selection.h
+++ clang-tools-extra/clangd/Selection.h
@@ -103,15 +103,15 @@
Selection Selected;
// Walk up the AST to get the DeclContext of this Node,
// which is not the node itself.
- const DeclContext& getDeclContext() const;
+ const DeclContext &getDeclContext() const;
// Printable node kind, like "CXXRecordDecl" or "AutoTypeLoc".
std::string kind() const;
// If this node is a wrapper with no syntax (e.g. implicit cast), return
// its contents. (If multiple wrappers are present, unwraps all of them).
- const Node& ignoreImplicit() const;
+ const Node &ignoreImplicit() const;
// If this node is inside a wrapper with no syntax (e.g. implicit cast),
// return that wrapper. (If multiple are present, unwraps all of them).
- const Node& outerImplicit() const;
+ const Node &outerImplicit() const;
};
// The most specific common ancestor of all the selected nodes.
// Returns nullptr if the common ancestor is the root.
Index: clang-tools-extra/clangd/Selection.cpp
===================================================================
--- clang-tools-extra/clangd/Selection.cpp
+++ clang-tools-extra/clangd/Selection.cpp
@@ -52,8 +52,7 @@
// On traversing an AST node, its token range is erased from the unclaimed set.
// The tokens actually removed are associated with that node, and hit-tested
// against the selection to determine whether the node is selected.
-template <typename T>
-class IntervalSet {
+template <typename T> class IntervalSet {
public:
IntervalSet(llvm::ArrayRef<T> Range) { UnclaimedRanges.insert(Range); }
@@ -78,7 +77,7 @@
--Overlap.first;
// ...unless B isn't selected at all.
if (Overlap.first->end() <= Claim.begin())
- ++Overlap.first;
+ ++Overlap.first;
}
if (Overlap.first == Overlap.second)
return Out;
@@ -118,8 +117,7 @@
};
// Disjoint sorted unclaimed ranges of expanded tokens.
- std::set<llvm::ArrayRef<T>, RangeLess>
- UnclaimedRanges;
+ std::set<llvm::ArrayRef<T>, RangeLess> UnclaimedRanges;
};
// Sentinel value for the selectedness of a node where we've seen no tokens yet.
@@ -142,12 +140,13 @@
Result = SelectionTree::Partial;
}
-
// SelectionTester can determine whether a range of tokens from the PP-expanded
// stream (corresponding to an AST node) is considered selected.
//
// When the tokens result from macro expansions, the appropriate tokens in the
// main file are examined (macro invocation or args). Similarly for #includes.
+// However, only the first expansion of a given spelled token is considered
+// selected.
//
// It tests each token in the range (not just the endpoints) as contiguous
// expanded tokens may not have contiguous spellings (with macros).
@@ -257,6 +256,11 @@
// Handle tokens that were passed as a macro argument.
SourceLocation ArgStart = SM.getTopMacroCallerLoc(StartLoc);
if (SM.getFileID(ArgStart) == SelFile) {
+ // If we've seen an occurrence of this macro argument before, don't
+ // consider it selected.
+ if (!SeenMacroCalls.insert(ArgStart).second) {
+ return NoTokens;
+ }
SourceLocation ArgEnd = SM.getTopMacroCallerLoc(Batch.back().location());
return testTokenRange(SM.getFileOffset(ArgStart),
SM.getFileOffset(ArgEnd));
@@ -316,6 +320,9 @@
};
std::vector<Tok> SpelledTokens;
FileID SelFile;
+ // This tracks the macro calls for which an expansion range has been tested.
+ // We only consider the first expansion range to be selected.
+ mutable llvm::SmallSet<SourceLocation, 4> SeenMacroCalls;
const SourceManager &SM;
};
@@ -343,7 +350,7 @@
}
#endif
-bool isImplicit(const Stmt* S) {
+bool isImplicit(const Stmt *S) {
// Some Stmts are implicit and shouldn't be traversed, but there's no
// "implicit" attribute on Stmt/Expr.
// Unwrap implicit casts first if present (other nodes too?).
@@ -600,7 +607,7 @@
// int (*[[s]])();
else if (auto *VD = llvm::dyn_cast<VarDecl>(D))
return VD->getLocation();
- } else if (const auto* CCI = N.get<CXXCtorInitializer>()) {
+ } else if (const auto *CCI = N.get<CXXCtorInitializer>()) {
// : [[b_]](42)
return CCI->getMemberLocation();
}
@@ -717,10 +724,10 @@
return Ancestor != Root ? Ancestor : nullptr;
}
-const DeclContext& SelectionTree::Node::getDeclContext() const {
- for (const Node* CurrentNode = this; CurrentNode != nullptr;
+const DeclContext &SelectionTree::Node::getDeclContext() const {
+ for (const Node *CurrentNode = this; CurrentNode != nullptr;
CurrentNode = CurrentNode->Parent) {
- if (const Decl* Current = CurrentNode->ASTNode.get<Decl>()) {
+ if (const Decl *Current = CurrentNode->ASTNode.get<Decl>()) {
if (CurrentNode != this)
if (auto *DC = dyn_cast<DeclContext>(Current))
return *DC;
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
