[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGdecdbc1155f5: [clangd] Use expansion location when the ref 
is inside macros. (authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480

Files:
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.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
@@ -811,6 +811,19 @@
 } // namespace ns
 int main() { [[^ns]]::Foo foo; }
   )cpp",
+
+  R"cpp(// Macros
+#define TYPE(X) X
+#define FOO Foo
+#define CAT(X, Y) X##Y
+class [[Fo^o]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -718,6 +718,29 @@
   HaveRanges(Header.ranges();
 }
 
+TEST_F(SymbolCollectorTest, RefsOnMacros) {
+  // Refs collected from SymbolCollector behave in the same way as
+  // AST-based xrefs.
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+#define TYPE(X) X
+#define FOO Foo
+#define CAT(X, Y) X##Y
+class [[Foo]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}
+  )");
+  CollectorOpts.RefFilter = RefKind::All;
+  runSymbolCollector(Header.code(), "");
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(Header.ranges();
+}
+
 TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
   CollectorOpts.RefFilter = RefKind::All;
   Annotations Header(R"(
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -276,10 +276,9 @@
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
   auto  = ASTCtx->getSourceManager();
-  auto SpellingLoc = SM.getSpellingLoc(Loc);
   if (Opts.CountReferences &&
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);
 
   auto ID = getSymbolID(ND);
@@ -312,9 +311,14 @@
   !shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
 return true;
   // Do not store references to main-file symbols.
+  // Unlike other fields, e.g. Symbols (which use spelling locations), we use
+  // file locations for references (as it aligns the behavior of clangd's
+  // AST-based xref).
+  // FIXME: we should try to use the file locations for other fields.
   if (CollectRef && !IsMainFileOnly && !isa(ND) &&
-  (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
-DeclRefs[ND].emplace_back(SpellingLoc, Roles);
+  (Opts.RefsInHeaders ||
+   SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
+DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
 return true;


Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -811,6 +811,19 @@
 } // namespace ns
 int main() { [[^ns]]::Foo foo; }
   )cpp",
+
+  R"cpp(// Macros
+#define TYPE(X) X
+#define FOO Foo
+#define CAT(X, Y) X##Y
+class [[Fo^o]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ 

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

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

merge_guards_bot added a comment.

Build result: pass - 60625 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 232853.
hokein added a comment.

add a fixme.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480

Files:
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.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
@@ -811,6 +811,19 @@
 } // namespace ns
 int main() { [[^ns]]::Foo foo; }
   )cpp",
+
+  R"cpp(// Macros
+#define TYPE(X) X
+#define FOO Foo
+#define CAT(X, Y) X##Y
+class [[Fo^o]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -718,6 +718,29 @@
   HaveRanges(Header.ranges();
 }
 
+TEST_F(SymbolCollectorTest, RefsOnMacros) {
+  // Refs collected from SymbolCollector behave in the same way as
+  // AST-based xrefs.
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+#define TYPE(X) X
+#define FOO Foo
+#define CAT(X, Y) X##Y
+class [[Foo]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}
+  )");
+  CollectorOpts.RefFilter = RefKind::All;
+  runSymbolCollector(Header.code(), "");
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(Header.ranges();
+}
+
 TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
   CollectorOpts.RefFilter = RefKind::All;
   Annotations Header(R"(
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -276,10 +276,9 @@
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
   auto  = ASTCtx->getSourceManager();
-  auto SpellingLoc = SM.getSpellingLoc(Loc);
   if (Opts.CountReferences &&
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);
 
   auto ID = getSymbolID(ND);
@@ -312,9 +311,14 @@
   !shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
 return true;
   // Do not store references to main-file symbols.
+  // Unlike other fields, e.g. Symbols (which use spelling locations), we use
+  // file locations for references (as it aligns the behavior of clangd's
+  // AST-based xref).
+  // FIXME: we should try to use the file locations for other fields.
   if (CollectRef && !IsMainFileOnly && !isa(ND) &&
-  (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
-DeclRefs[ND].emplace_back(SpellingLoc, Roles);
+  (Opts.RefsInHeaders ||
+   SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
+DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
 return true;


Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -811,6 +811,19 @@
 } // namespace ns
 int main() { [[^ns]]::Foo foo; }
   )cpp",
+
+  R"cpp(// Macros
+#define TYPE(X) X
+#define FOO Foo
+#define CAT(X, Y) X##Y
+class [[Fo^o]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -718,6 +718,29 @@
   HaveRanges(Header.ranges();
 }
 

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:280
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > hokein wrote:
> > > > ilya-biryukov wrote:
> > > > > We're using `getSpellingLoc` here and `getFileLoc` later. Why not use 
> > > > > `getFileLoc` everywhere?
> > > > > 
> > > > > Having a variable (similar to the `SpellingLoc` we had before) and 
> > > > > calling `getFileLoc` only once also seems preferable.
> > > > > We're using getSpellingLoc here and getFileLoc later. Why not use 
> > > > > getFileLoc everywhere?
> > > > 
> > > > There are two things in SymbolCollector:
> > > > - symbols & ranking signals, we use spelling location for them, the 
> > > > code is part of this, `ReferencedDecls` is used to calculate the ranking
> > > > - references
> > > > 
> > > > this patch only targets the reference part (changing the loc here would 
> > > > break many assumptions I think, and there was a failure test).
> > > - What are the assumptions that it will break?
> > > - What is rationale for using spelling locations for ranking and file 
> > > location for references?
> > > 
> > > It would be nice to have this spelled out somewhere in the code, too. 
> > > Currently this looks like an accidental inconsistency. Especially given 
> > > that `getFileLoc` and `getSpellingLoc` are often the same.
> > Added comments to clarify the difference between references and other 
> > fields. 
> The comment still does not explain *why* we do this.
> Why not use the same type of locations for everything?
Update after offline discussion:
there does not seem to be any good reason to use spelling locations and not 
file locations here. Some reference counts will be affected, but only those 
that end up being spelled in the macros inside the main file and used outside 
the main file. Which is very rare and shouldn't affect completion ranking much.

We should definitely try switching to file locations everywhere, but that means 
doing more changes. Therefore, it's more appropriate to do it in the follow-up 
change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:280
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);

hokein wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > ilya-biryukov wrote:
> > > > We're using `getSpellingLoc` here and `getFileLoc` later. Why not use 
> > > > `getFileLoc` everywhere?
> > > > 
> > > > Having a variable (similar to the `SpellingLoc` we had before) and 
> > > > calling `getFileLoc` only once also seems preferable.
> > > > We're using getSpellingLoc here and getFileLoc later. Why not use 
> > > > getFileLoc everywhere?
> > > 
> > > There are two things in SymbolCollector:
> > > - symbols & ranking signals, we use spelling location for them, the code 
> > > is part of this, `ReferencedDecls` is used to calculate the ranking
> > > - references
> > > 
> > > this patch only targets the reference part (changing the loc here would 
> > > break many assumptions I think, and there was a failure test).
> > - What are the assumptions that it will break?
> > - What is rationale for using spelling locations for ranking and file 
> > location for references?
> > 
> > It would be nice to have this spelled out somewhere in the code, too. 
> > Currently this looks like an accidental inconsistency. Especially given 
> > that `getFileLoc` and `getSpellingLoc` are often the same.
> Added comments to clarify the difference between references and other fields. 
The comment still does not explain *why* we do this.
Why not use the same type of locations for everything?



Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:659
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}

hokein wrote:
> ilya-biryukov wrote:
> > Previously we would not report any location at all in that case, right?
> > Not sure how common this is, hope this won't blow up our index size too 
> > much. No need to change anything now, but we should be ready to revert if 
> > needed.
> > 
> > Worth putting a comment here that AST-based XRefs behave in the same way. 
> > (And maybe adding a test there, if there isn't one already)
> Yes, I measure the memory usage before vs after, it increased ~5% memory 
> usage.
On LLVM? This doesn't look too bad, but probably worth mentioning it in the 
patch description


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

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

merge_guards_bot added a comment.

Build result: pass - 60296 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:280
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);

ilya-biryukov wrote:
> hokein wrote:
> > ilya-biryukov wrote:
> > > We're using `getSpellingLoc` here and `getFileLoc` later. Why not use 
> > > `getFileLoc` everywhere?
> > > 
> > > Having a variable (similar to the `SpellingLoc` we had before) and 
> > > calling `getFileLoc` only once also seems preferable.
> > > We're using getSpellingLoc here and getFileLoc later. Why not use 
> > > getFileLoc everywhere?
> > 
> > There are two things in SymbolCollector:
> > - symbols & ranking signals, we use spelling location for them, the code is 
> > part of this, `ReferencedDecls` is used to calculate the ranking
> > - references
> > 
> > this patch only targets the reference part (changing the loc here would 
> > break many assumptions I think, and there was a failure test).
> - What are the assumptions that it will break?
> - What is rationale for using spelling locations for ranking and file 
> location for references?
> 
> It would be nice to have this spelled out somewhere in the code, too. 
> Currently this looks like an accidental inconsistency. Especially given that 
> `getFileLoc` and `getSpellingLoc` are often the same.
Added comments to clarify the difference between references and other fields. 



Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:659
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}

ilya-biryukov wrote:
> Previously we would not report any location at all in that case, right?
> Not sure how common this is, hope this won't blow up our index size too much. 
> No need to change anything now, but we should be ready to revert if needed.
> 
> Worth putting a comment here that AST-based XRefs behave in the same way. 
> (And maybe adding a test there, if there isn't one already)
Yes, I measure the memory usage before vs after, it increased ~5% memory usage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 230905.
hokein marked 2 inline comments as done.
hokein added a comment.

address comments

- add related test for ast-based xrefs
- add comments to clarify using different location for symbols/references.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480

Files:
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.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
@@ -796,6 +796,19 @@
 } // namespace ns
 int main() { [[^ns]]::Foo foo; }
   )cpp",
+
+  R"cpp(// Macros
+#define TYPE(X) X
+#define FOO Foo
+#define CAT(X, Y) X##Y
+class [[Fo^o]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -644,6 +644,29 @@
   HaveRanges(Header.ranges();
 }
 
+TEST_F(SymbolCollectorTest, RefsOnMacros) {
+  // Refs collected from SymbolCollector behave in the same way as
+  // AST-based xrefs.
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+#define TYPE(X) X
+#define FOO Foo
+#define CAT(X, Y) X##Y
+class [[Foo]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}
+  )");
+  CollectorOpts.RefFilter = RefKind::All;
+  runSymbolCollector(Header.code(), "");
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(Header.ranges();
+}
+
 TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
   CollectorOpts.RefFilter = RefKind::All;
   Annotations Header(R"(
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -275,10 +275,9 @@
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
   auto  = ASTCtx->getSourceManager();
-  auto SpellingLoc = SM.getSpellingLoc(Loc);
   if (Opts.CountReferences &&
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);
 
   auto ID = getSymbolID(ND);
@@ -311,9 +310,13 @@
   !shouldCollectSymbol(*ND, *ASTCtx, Opts, IsMainFileOnly))
 return true;
   // Do not store references to main-file symbols.
+  // Unlike other fields, e.g. Symbols (which use spelling locations), we use
+  // expansion locations for references comming from macros (as it aligns the
+  // behavior of clangd's AST-based xref).
   if (CollectRef && !IsMainFileOnly && !isa(ND) &&
-  (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
-DeclRefs[ND].emplace_back(SpellingLoc, Roles);
+  (Opts.RefsInHeaders ||
+   SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
+DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
 return true;


Index: clang-tools-extra/clangd/unittests/XRefsTests.cpp
===
--- clang-tools-extra/clangd/unittests/XRefsTests.cpp
+++ clang-tools-extra/clangd/unittests/XRefsTests.cpp
@@ -796,6 +796,19 @@
 } // namespace ns
 int main() { [[^ns]]::Foo foo; }
   )cpp",
+
+  R"cpp(// Macros
+#define TYPE(X) X
+#define FOO Foo
+#define CAT(X, Y) X##Y
+class [[Fo^o]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}
+  )cpp",
   };
   for (const char *Test : Tests) {
 Annotations T(Test);
Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ 

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:659
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}

Previously we would not report any location at all in that case, right?
Not sure how common this is, hope this won't blow up our index size too much. 
No need to change anything now, but we should be ready to revert if needed.

Worth putting a comment here that AST-based XRefs behave in the same way. (And 
maybe adding a test there, if there isn't one already)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:280
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);

hokein wrote:
> ilya-biryukov wrote:
> > We're using `getSpellingLoc` here and `getFileLoc` later. Why not use 
> > `getFileLoc` everywhere?
> > 
> > Having a variable (similar to the `SpellingLoc` we had before) and calling 
> > `getFileLoc` only once also seems preferable.
> > We're using getSpellingLoc here and getFileLoc later. Why not use 
> > getFileLoc everywhere?
> 
> There are two things in SymbolCollector:
> - symbols & ranking signals, we use spelling location for them, the code is 
> part of this, `ReferencedDecls` is used to calculate the ranking
> - references
> 
> this patch only targets the reference part (changing the loc here would break 
> many assumptions I think, and there was a failure test).
- What are the assumptions that it will break?
- What is rationale for using spelling locations for ranking and file location 
for references?

It would be nice to have this spelled out somewhere in the code, too. Currently 
this looks like an accidental inconsistency. Especially given that `getFileLoc` 
and `getSpellingLoc` are often the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

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

merge_guards_bot added a comment.

Build result: pass - 60164 tests passed, 0 failed and 731 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:280
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);

ilya-biryukov wrote:
> We're using `getSpellingLoc` here and `getFileLoc` later. Why not use 
> `getFileLoc` everywhere?
> 
> Having a variable (similar to the `SpellingLoc` we had before) and calling 
> `getFileLoc` only once also seems preferable.
> We're using getSpellingLoc here and getFileLoc later. Why not use getFileLoc 
> everywhere?

There are two things in SymbolCollector:
- symbols & ranking signals, we use spelling location for them, the code is 
part of this, `ReferencedDecls` is used to calculate the ranking
- references

this patch only targets the reference part (changing the loc here would break 
many assumptions I think, and there was a failure test).



Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:655
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;

ilya-biryukov wrote:
> Could you also check:
> 1. Multi-level macro arguments `TYPE(TYPE(FOO))`
> 2. Concatenated tokens as identifiers, e.g. coming from `#define CAT(X, Y) 
> X##Y`
Added tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Haojian Wu via Phabricator via cfe-commits]

hokein updated this revision to Diff 230249.
hokein marked 3 inline comments as done.
hokein added a comment.

add more tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480

Files:
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -644,6 +644,27 @@
   HaveRanges(Header.ranges();
 }
 
+TEST_F(SymbolCollectorTest, RefsOnMacros) {
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+#define TYPE(X) X
+#define FOO Foo
+#define CAT(X, Y) X##Y
+class [[Foo]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}
+  )");
+  CollectorOpts.RefFilter = RefKind::All;
+  runSymbolCollector(Header.code(), "");
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(Header.ranges();
+}
+
 TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
   CollectorOpts.RefFilter = RefKind::All;
   Annotations Header(R"(
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -275,10 +275,9 @@
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
   auto  = ASTCtx->getSourceManager();
-  auto SpellingLoc = SM.getSpellingLoc(Loc);
   if (Opts.CountReferences &&
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);
 
   auto ID = getSymbolID(ND);
@@ -312,8 +311,9 @@
 return true;
   // Do not store references to main-file symbols.
   if (CollectRef && !IsMainFileOnly && !isa(ND) &&
-  (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
-DeclRefs[ND].emplace_back(SpellingLoc, Roles);
+  (Opts.RefsInHeaders ||
+   SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
+DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
 return true;


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -644,6 +644,27 @@
   HaveRanges(Header.ranges();
 }
 
+TEST_F(SymbolCollectorTest, RefsOnMacros) {
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+#define TYPE(X) X
+#define FOO Foo
+#define CAT(X, Y) X##Y
+class [[Foo]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+  TYPE(TYPE([[Foo]])) foo3;
+  [[CAT]](Fo, o) foo4;
+}
+  )");
+  CollectorOpts.RefFilter = RefKind::All;
+  runSymbolCollector(Header.code(), "");
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(Header.ranges();
+}
+
 TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
   CollectorOpts.RefFilter = RefKind::All;
   Annotations Header(R"(
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -275,10 +275,9 @@
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
   auto  = ASTCtx->getSourceManager();
-  auto SpellingLoc = SM.getSpellingLoc(Loc);
   if (Opts.CountReferences &&
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);
 
   auto ID = getSymbolID(ND);
@@ -312,8 +311,9 @@
 return true;
   // Do not store references to main-file symbols.
   if (CollectRef && !IsMainFileOnly && !isa(ND) &&
-  (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
-DeclRefs[ND].emplace_back(SpellingLoc, Roles);
+  (Opts.RefsInHeaders ||
+   SM.getFileID(SM.getFileLoc(Loc)) == 

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

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

merge_guards_bot added a comment.

Build result: pass - 60164 tests passed, 0 failed and 731 were skipped.
Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:280
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);

We're using `getSpellingLoc` here and `getFileLoc` later. Why not use 
`getFileLoc` everywhere?

Having a variable (similar to the `SpellingLoc` we had before) and calling 
`getFileLoc` only once also seems preferable.



Comment at: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp:655
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;

Could you also check:
1. Multi-level macro arguments `TYPE(TYPE(FOO))`
2. Concatenated tokens as identifiers, e.g. coming from `#define CAT(X, Y) X##Y`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70480



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

[PATCH] D70480: [clangd] Use expansion location when the ref is inside macros.

[Haojian Wu via Phabricator via cfe-commits]

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

Previously, xrefs has inconsistent behavior when the reference is inside
macro body:

- AST-based xrefs (for main file) uses the expansion location;
- our index uses the spelling location;

This patch makes our index use expansion location, which is consistent with
AST-based xrefs, and kythe as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D70480

Files:
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -644,6 +644,24 @@
   HaveRanges(Header.ranges();
 }
 
+TEST_F(SymbolCollectorTest, RefsOnMacros) {
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+#define TYPE(X) X
+#define FOO Foo
+class [[Foo]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+}
+  )");
+  CollectorOpts.RefFilter = RefKind::All;
+  runSymbolCollector(Header.code(), "");
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(Header.ranges();
+}
+
 TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
   CollectorOpts.RefFilter = RefKind::All;
   Annotations Header(R"(
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -275,10 +275,9 @@
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
   auto  = ASTCtx->getSourceManager();
-  auto SpellingLoc = SM.getSpellingLoc(Loc);
   if (Opts.CountReferences &&
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);
 
   auto ID = getSymbolID(ND);
@@ -312,8 +311,9 @@
 return true;
   // Do not store references to main-file symbols.
   if (CollectRef && !IsMainFileOnly && !isa(ND) &&
-  (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
-DeclRefs[ND].emplace_back(SpellingLoc, Roles);
+  (Opts.RefsInHeaders ||
+   SM.getFileID(SM.getFileLoc(Loc)) == SM.getMainFileID()))
+DeclRefs[ND].emplace_back(SM.getFileLoc(Loc), Roles);
   // Don't continue indexing if this is a mere reference.
   if (IsOnlyRef)
 return true;


Index: clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
===
--- clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
+++ clang-tools-extra/clangd/unittests/SymbolCollectorTests.cpp
@@ -644,6 +644,24 @@
   HaveRanges(Header.ranges();
 }
 
+TEST_F(SymbolCollectorTest, RefsOnMacros) {
+  CollectorOpts.RefFilter = RefKind::All;
+  CollectorOpts.RefsInHeaders = true;
+  Annotations Header(R"(
+#define TYPE(X) X
+#define FOO Foo
+class [[Foo]] {};
+void test() {
+  TYPE([[Foo]]) foo;
+  [[FOO]] foo2;
+}
+  )");
+  CollectorOpts.RefFilter = RefKind::All;
+  runSymbolCollector(Header.code(), "");
+  EXPECT_THAT(Refs, Contains(Pair(findSymbol(Symbols, "Foo").ID,
+  HaveRanges(Header.ranges();
+}
+
 TEST_F(SymbolCollectorTest, HeaderAsMainFile) {
   CollectorOpts.RefFilter = RefKind::All;
   Annotations Header(R"(
Index: clang-tools-extra/clangd/index/SymbolCollector.cpp
===
--- clang-tools-extra/clangd/index/SymbolCollector.cpp
+++ clang-tools-extra/clangd/index/SymbolCollector.cpp
@@ -275,10 +275,9 @@
   // Mark D as referenced if this is a reference coming from the main file.
   // D may not be an interesting symbol, but it's cheaper to check at the end.
   auto  = ASTCtx->getSourceManager();
-  auto SpellingLoc = SM.getSpellingLoc(Loc);
   if (Opts.CountReferences &&
   (Roles & static_cast(index::SymbolRole::Reference)) &&
-  SM.getFileID(SpellingLoc) == SM.getMainFileID())
+  SM.getFileID(SM.getSpellingLoc(Loc)) == SM.getMainFileID())
 ReferencedDecls.insert(ND);
 
   auto ID = getSymbolID(ND);
@@ -312,8 +311,9 @@
 return true;
   // Do not store references to main-file symbols.
   if (CollectRef && !IsMainFileOnly && !isa(ND) &&
-  (Opts.RefsInHeaders || SM.getFileID(SpellingLoc) == SM.getMainFileID()))
-