[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Nathan Ridge via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
nridge marked 4 inline comments as done.
Closed by commit rGe70a9f385025: [clangd] Handle go-to-definition in macro 
invocations where the target appears… (authored by nridge).

Changed prior to commit:
  https://reviews.llvm.org/D72041?vs=247106=248005#toc

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
@@ -338,7 +338,18 @@
#define ADDRESSOF(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
@@ -415,7 +415,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;
@@ -508,7 +508,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);
@@ -517,15 +518,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).
@@ -542,7 +536,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
@@ -64,6 +64,9 @@
 //
 // SelectionTree tries to behave sensibly in the presence of macros, but does
 // not model any preprocessor concepts: the output is a subset of the AST.
+// When a macro argument is specifically selected, only its first expansion is
+// selected in the AST. (Returning a selection forest is unreasonably difficult
+// for callers to handle correctly.)
 //
 // Comments, directives and whitespace are completely ignored.
 // Semicolons are also ignored, as the AST generally does not model them well.
@@ -127,15 +130,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 () 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 () 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 () const;
   };
   // 

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.



Comment at: clang-tools-extra/clangd/Selection.cpp:287
 if (SM.getFileID(ArgStart) == SelFile) {
-  SourceLocation ArgEnd = SM.getTopMacroCallerLoc(Batch.back().location());
-  return testTokenRange(SM.getFileOffset(ArgStart),
-SM.getFileOffset(ArgEnd));
+  if (isFirstExpansion(FID, ArgStart, SM)) {
+SourceLocation ArgEnd =

nridge wrote:
> sammccall wrote:
> > when false is the fallthrough to handling as if part of the macro body 
> > deliberate?
> > 
> > Thinking about it I suppose either that or returning NoTokens works, but 
> > please document it, e.g. `} else { /* fall through and treat as part of the 
> > macro body */}`
> It is deliberate. In fact, it is intended to address [this previous 
> comment](https://reviews.llvm.org/D72041#inline-663000). Please let me know 
> if I've misunderstood and the solution doesn't match the request.
> 
> I'll add a comment as you suggested.
Yeah I think the behavior is great, as is the implementation :-) just a little 
surprising.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked an inline comment as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/Selection.cpp:287
 if (SM.getFileID(ArgStart) == SelFile) {
-  SourceLocation ArgEnd = SM.getTopMacroCallerLoc(Batch.back().location());
-  return testTokenRange(SM.getFileOffset(ArgStart),
-SM.getFileOffset(ArgEnd));
+  if (isFirstExpansion(FID, ArgStart, SM)) {
+SourceLocation ArgEnd =

sammccall wrote:
> when false is the fallthrough to handling as if part of the macro body 
> deliberate?
> 
> Thinking about it I suppose either that or returning NoTokens works, but 
> please document it, e.g. `} else { /* fall through and treat as part of the 
> macro body */}`
It is deliberate. In fact, it is intended to address [this previous 
comment](https://reviews.llvm.org/D72041#inline-663000). Please let me know if 
I've misunderstood and the solution doesn't match the request.

I'll add a comment as you suggested.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Awesome, thanks for fixing this!




Comment at: clang-tools-extra/clangd/Selection.cpp:261
+  // consider it selected.
+  if (!SeenMacroCalls.insert(ArgStart).second) {
+return NoTokens;

nridge wrote:
> sammccall wrote:
> > sammccall wrote:
> > > Given the following program:
> > > ```
> > > #define SQUARE(x) x * x;
> > > int four = [[SQUARE(2)]];
> > > ```
> > > We're going to now report the binary operator and one of the operands as 
> > > selected and not the other, which doesn't seem desirable.
> > > 
> > > I think we want to accept macro-selected || arg-selected, so probably 
> > > doing the current "non-argument macro expansion" first unconditionally or 
> > > factoring it out into a function.
> > > 
> > > This will change the behavior of `int four = [[SQUARE]](2)` to consider 
> > > the literal children selected too, I think this is fine.
> > I don't think it's a good idea to add hidden state and side-effects to 
> > testChunk() - it breaks a lot of assumptions that help reason about the 
> > code, and using `mutable` hides the violation of them.
> > (And a possible source of bugs - this is first in traversal order rather 
> > than first in source order - these are mostly but IIRC not always the same).
> > 
> > Instead I think you can do this statelessly: from the top-level spelling 
> > location, walk down with `SM.getMacroArgExpandedLocation` until you hit the 
> > target FileID (this is the first-expansion of first-expansion of 
> > first-expansion...) or the FileID stops changing (you've reached the 
> > innermost macro invocation, and your target location was on a different 
> > branch).
> I agree that adding state is not great. I thought it was icky as I was 
> writing it, I just couldn't think of an alternative. Thank you for suggesting 
> one!
> 
> I implemented what you suggested, and it seems to work. I did want to ask a 
> clarifying question to make sure I understand correctly: when an argument 
> occurs multiple times in a macro exapnsion, the occurrences will have 
> distinct `FileID`s (as opposed just different offsets in the same macro 
> `FileID`)?
That's right. My mental model is:
 - a TU is a sequence of expanded (i.e. post-PP) tokens
 - a FileID always corresponds to a single subrange of these expanded tokens...
 - ...with holes in it for child FileIDs. Parents/children nest properly.

One of the things I really like about the syntax::TokenBuffer API is that it 
exposes this structure.



Comment at: clang-tools-extra/clangd/Selection.cpp:166
+if (SM.getFileID(Next) == SM.getFileID(Prev)) {
+  break;
+}

nit: return false directly?



Comment at: clang-tools-extra/clangd/Selection.cpp:287
 if (SM.getFileID(ArgStart) == SelFile) {
-  SourceLocation ArgEnd = SM.getTopMacroCallerLoc(Batch.back().location());
-  return testTokenRange(SM.getFileOffset(ArgStart),
-SM.getFileOffset(ArgEnd));
+  if (isFirstExpansion(FID, ArgStart, SM)) {
+SourceLocation ArgEnd =

when false is the fallthrough to handling as if part of the macro body 
deliberate?

Thinking about it I suppose either that or returning NoTokens works, but please 
document it, e.g. `} else { /* fall through and treat as part of the macro body 
*/}`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Nathan Ridge via Phabricator via cfe-commits]

nridge marked 4 inline comments as done.
nridge added inline comments.



Comment at: clang-tools-extra/clangd/Selection.cpp:261
+  // consider it selected.
+  if (!SeenMacroCalls.insert(ArgStart).second) {
+return NoTokens;

sammccall wrote:
> sammccall wrote:
> > Given the following program:
> > ```
> > #define SQUARE(x) x * x;
> > int four = [[SQUARE(2)]];
> > ```
> > We're going to now report the binary operator and one of the operands as 
> > selected and not the other, which doesn't seem desirable.
> > 
> > I think we want to accept macro-selected || arg-selected, so probably doing 
> > the current "non-argument macro expansion" first unconditionally or 
> > factoring it out into a function.
> > 
> > This will change the behavior of `int four = [[SQUARE]](2)` to consider the 
> > literal children selected too, I think this is fine.
> I don't think it's a good idea to add hidden state and side-effects to 
> testChunk() - it breaks a lot of assumptions that help reason about the code, 
> and using `mutable` hides the violation of them.
> (And a possible source of bugs - this is first in traversal order rather than 
> first in source order - these are mostly but IIRC not always the same).
> 
> Instead I think you can do this statelessly: from the top-level spelling 
> location, walk down with `SM.getMacroArgExpandedLocation` until you hit the 
> target FileID (this is the first-expansion of first-expansion of 
> first-expansion...) or the FileID stops changing (you've reached the 
> innermost macro invocation, and your target location was on a different 
> branch).
I agree that adding state is not great. I thought it was icky as I was writing 
it, I just couldn't think of an alternative. Thank you for suggesting one!

I implemented what you suggested, and it seems to work. I did want to ask a 
clarifying question to make sure I understand correctly: when an argument 
occurs multiple times in a macro exapnsion, the occurrences will have distinct 
`FileID`s (as opposed just different offsets in the same macro `FileID`)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 247106.
nridge marked an inline comment as done.
nridge added a comment.

Address review comments


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
@@ -329,7 +329,18 @@
#define ADDRESSOF(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
@@ -415,7 +415,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;
@@ -508,7 +508,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);
@@ -517,15 +518,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).
@@ -542,7 +536,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
@@ -64,6 +64,9 @@
 //
 // SelectionTree tries to behave sensibly in the presence of macros, but does
 // not model any preprocessor concepts: the output is a subset of the AST.
+// When a macro argument is specifically selected, only its first expansion is
+// selected in the AST. (Returning a selection forest is unreasonably difficult
+// for callers to handle correctly.)
 //
 // Comments, directives and whitespace are completely ignored.
 // Semicolons are also ignored, as the AST generally does not model them well.
@@ -127,15 +130,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 () 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 () 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 () 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

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for fixing this. I think it could maybe use a further tweak, LMK if I'm 
missing something as my intuition around macros is pretty bad.




Comment at: clang-tools-extra/clangd/Selection.cpp:261
+  // consider it selected.
+  if (!SeenMacroCalls.insert(ArgStart).second) {
+return NoTokens;

Given the following program:
```
#define SQUARE(x) x * x;
int four = [[SQUARE(2)]];
```
We're going to now report the binary operator and one of the operands as 
selected and not the other, which doesn't seem desirable.

I think we want to accept macro-selected || arg-selected, so probably doing the 
current "non-argument macro expansion" first unconditionally or factoring it 
out into a function.

This will change the behavior of `int four = [[SQUARE]](2)` to consider the 
literal children selected too, I think this is fine.



Comment at: clang-tools-extra/clangd/Selection.cpp:261
+  // consider it selected.
+  if (!SeenMacroCalls.insert(ArgStart).second) {
+return NoTokens;

sammccall wrote:
> Given the following program:
> ```
> #define SQUARE(x) x * x;
> int four = [[SQUARE(2)]];
> ```
> We're going to now report the binary operator and one of the operands as 
> selected and not the other, which doesn't seem desirable.
> 
> I think we want to accept macro-selected || arg-selected, so probably doing 
> the current "non-argument macro expansion" first unconditionally or factoring 
> it out into a function.
> 
> This will change the behavior of `int four = [[SQUARE]](2)` to consider the 
> literal children selected too, I think this is fine.
I don't think it's a good idea to add hidden state and side-effects to 
testChunk() - it breaks a lot of assumptions that help reason about the code, 
and using `mutable` hides the violation of them.
(And a possible source of bugs - this is first in traversal order rather than 
first in source order - these are mostly but IIRC not always the same).

Instead I think you can do this statelessly: from the top-level spelling 
location, walk down with `SM.getMacroArgExpandedLocation` until you hit the 
target FileID (this is the first-expansion of first-expansion of 
first-expansion...) or the FileID stops changing (you've reached the innermost 
macro invocation, and your target location was on a different branch).



Comment at: clang-tools-extra/clangd/Selection.h:58
 // SelectionTree tries to behave sensibly in the presence of macros, but does
 // not model any preprocessor concepts: the output is a subset of the AST.
 //

Worth mentioning the new behavior here, IMO. e.g.

```
When a macro argument is specifically selected, only its first expansion is 
selected in the AST.
(Returning a selection forest is unreasonably difficult for callers to handle 
correctly).
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61848 tests passed, 0 failed 
and 781 were skipped.

{icon question-circle color=gray} clang-tidy: unknown.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Nathan Ridge via Phabricator via cfe-commits]

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) 
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 () 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 () 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 () 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 
-class IntervalSet {
+template  class IntervalSet {
 public:
   IntervalSet(llvm::ArrayRef Range) { UnclaimedRanges.insert(Range); }
 
@@ 

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Nathan Ridge via Phabricator via cfe-commits]

nridge added a comment.

In D72041#1800189 , @sammccall wrote:

> - the allSelectedNodes API on SelectionTree feels non-orthogonal: it's only 
> meaningful here because the input happened to be a point and thus touches a 
> single token and therefore node. If the input was a range, then this API 
> doesn't help, because it loses the sense of "topmost" node.


Good point!

> - maybe the function we want walks down the tree as if finding the common 
> ancestor, but may descend to return the roots of multiple subtrees if they 
> are located in distinct expansions of the same macro arg
> - or maybe we just want to treat non-first expansions as not-selected? That 
> would about this issue I think

I'm thinking of going with the second suggestion, because it doesn't require 
changes to the SelectionTree API, and therefore addresses this concern:

> - it seems unfortunate to solve this mostly in xrefs rather than mostly in 
> SelectionTree. All the other clients potentially want this behavior.

without touching all the clients of SelectionTree.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

I like the goal here, a few high-level comments:

- it seems unfortunate to solve this mostly in xrefs rather than mostly in 
SelectionTree. All the other clients potentially want this behavior.
- the allSelectedNodes API on SelectionTree feels non-orthogonal: it's only 
meaningful here because the input happened to be a point and thus touches a 
single token and therefore node. If the input was a range, then this API 
doesn't help, because it loses the sense of "topmost" node.
- maybe the function we want walks down the tree as if finding the common 
ancestor, but may descend to return the roots of multiple subtrees if they are 
located in distinct expansions of the same macro arg
- or maybe we just want to treat non-first expansions as not-selected? That 
would about this issue I think

(BTW, Haojian is out the first few weeks of Jan. I'm still technically OOO, 
back later this week)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

{icon check-circle color=green} Unit tests: pass. 61162 tests passed, 0 failed 
and 728 were skipped.

{icon check-circle color=green} clang-tidy: pass.

{icon check-circle color=green} clang-format: pass.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72041



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added a reviewer: hokein.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, 
MaskRay, ilya-biryukov.
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D72041

Files:
  clang-tools-extra/clangd/Selection.cpp
  clang-tools-extra/clangd/Selection.h
  clang-tools-extra/clangd/XRefs.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
@@ -349,7 +349,18 @@
#define ADDRESSOF(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/XRefs.cpp
===
--- clang-tools-extra/clangd/XRefs.cpp
+++ clang-tools-extra/clangd/XRefs.cpp
@@ -140,6 +140,28 @@
 auto Decls = targetDecl(N->ASTNode, Relations);
 Result.assign(Decls.begin(), Decls.end());
   }
+  // If the common ancestor didn't have a target, the individual selected
+  // nodes still might. This can happen if the selection is in a macro
+  // invocation and is over a token that appears in multiple places in
+  // the macro expansion. As long as all occurrences map to the same target,
+  // return that target.
+  if (Result.empty()) {
+auto SelectedNodes = Selection.allSelectedNodes();
+if (SelectedNodes.size() <= 1)
+  return {};
+for (const SelectionTree::Node *N : SelectedNodes) {
+  auto Decls = targetDecl(N->ASTNode, Relations);
+  if (Decls.size() != 1) {
+return Result;
+  }
+  auto Candidate = Decls[0];
+  if (Result.empty()) {
+Result.push_back(Decls[0]);
+  } else if (Result[0] != Candidate) {
+return {};
+  }
+}
+  }
   return Result;
 }
 
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 () 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 () 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 () const;
   };
   // The most specific common ancestor of all the selected nodes.
   // Returns nullptr if the common ancestor is the root.
@@ -119,6 +119,9 @@
   const Node *commonAncestor() const;
   // The selection node corresponding to TranslationUnitDecl.
   const Node () const { return *Root; }
+  // Get all individual selected nodes, in case the caller wants something
+  // other than their common ancestor.
+  std::vector allSelectedNodes() const;
 
 private:
   std::deque Nodes; // Stable-pointer storage.
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 
-class IntervalSet {
+template  class IntervalSet {
 public:
   IntervalSet(llvm::ArrayRef 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, RangeLess>
-  UnclaimedRanges;
+  std::set, RangeLess> UnclaimedRanges;
 };
 
 // Sentinel value for