[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<unsigned>(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