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