[PATCH] D38639: [clangd] #include statements support for Open definition

[Marc-Andre Laperle via Phabricator via cfe-commits]

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