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