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

[Ilya Biryukov via Phabricator via cfe-commits]

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