ilya-biryukov added a comment.
@malaperle, hi! Both new diff and updating this works, so feel free the one
that suits you best. I tend to look over the whole change on each new round of
reviews anyway.
================
Comment at: clangd/ClangdUnit.cpp:113
+ CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+ : IRM(IRM) {}
----------------
malaperle wrote:
> ilya-biryukov wrote:
> > ilya-biryukov wrote:
> > > Let's create a new empty map inside this class and have a
> > > `takeIncludeReferences` method (similar to `TopLevelDeclIDs` and
> > > `takeTopLevelDeclIDs`)
> > This comment is not addressed yet, but marked as done.
> As mentioned below, the idea was to have a single map being appended to,
> without having to merge two separate maps. However, I can change the code so
> that two separate maps are built and merged if you prefer.
>
> But I'm not so clear if that's what you'd prefer:
>
> > You copy the map for preamble and then append to it in
> > CppFilePreambleCallbacks? That also LG, we should not have many references
> > there anyway.
>
> It's not meant to have any copy. The idea was to create a single
> IncludeReferenceMap in CppFile::deferRebuild, populate it with both preamble
> and non-preamble include references and std::move it around for later use
> (stored in ParsedAST).
We can't have a single map because AST is rebuilt more often than the Preamble,
so we have two options:
- Store a map for the preamble separately, copy it when we need to rebuild the
AST and append references from the AST to the new instance of the map.
- Store two maps: one contains references only from the Preamble, the other one
from the AST.
I think both are fine, since the copy of the map will be cheap anyway, as we
only store a list of includes inside the main file.
================
Comment at: clangd/ClangdUnit.cpp:281
+ std::shared_ptr<PCHContainerOperations> PCHs,
+ IntrusiveRefCntPtr<vfs::FileSystem> VFS, IncludeReferenceMap IRM) {
----------------
malaperle wrote:
> ilya-biryukov wrote:
> > Don't we already store the map we need in `PreambleData`? Why do we need an
> > extra `IRM` parameter?
> Since the map will be now stored in ParsedAST and the instance doesn't exist
> yet, we need to keep the IRM parameter. I noticed that it wasn't being
> std::move'd though.
That looks fine. Also see the other comment on why we need to copy the map from
the Preamble, and not `std::move` it.
================
Comment at: clangd/XRefs.cpp:185
+
+ if ((unsigned)R.start.line ==
+ SourceMgr.getSpellingLineNumber(
----------------
malaperle wrote:
> ilya-biryukov wrote:
> > why do we need to convert to unsigned? To slience compiler warnings?
> Yes, "line" from the protocol is signed, whereas getSpellingColumn/lineNumber
> returns unsigned. I'll extract another var for the line number and cast both
> to int instead to have less casts and make the condition smaller.
Can we maybe convert to `clangd::Position` using the helper methods first and
do the comparison of two `clangd::Position`s?
Comparing between `clangd::Position` and clang's line/column numbers is a
common source of off-by-one errors in clangd.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
