malaperle added a comment.
@ilya-biryukov Hi! I'll be updating William's patches that were in progress. I
just have a few comments/question before I send a new update. (I also don't
know if I can update this diff or I have to create a new diff on Phabricator...
I guess we'll see!!).
================
Comment at: clangd/ClangdUnit.cpp:113
+ CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+ : IRM(IRM) {}
----------------
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).
================
Comment at: clangd/ClangdUnit.cpp:281
+ std::shared_ptr<PCHContainerOperations> PCHs,
+ IntrusiveRefCntPtr<vfs::FileSystem> VFS, IncludeReferenceMap IRM) {
----------------
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.
================
Comment at: clangd/XRefs.cpp:185
+
+ if ((unsigned)R.start.line ==
+ SourceMgr.getSpellingLineNumber(
----------------
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
