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:
> 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
