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