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);
----------------
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
