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

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

malaperle added inline comments.


================
Comment at: clangd/ClangdUnit.cpp:113
 
+  CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+      : IRM(IRM) {}
----------------
ilya-biryukov wrote:
> 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.
> We can't have a single map because AST is rebuilt more often than the Preamble, so we have two options:

Doh! Indeed.

OK so I added the map in PreambleData, this one will say every reparse unless the preamble changes. When the AST is built/rebuilt, the map is copied (or empty) and includes from the AST are appended to it then stored in ParsedAST (option #1?).


================
Comment at: clangd/XRefs.cpp:185
+
+      if ((unsigned)R.start.line ==
+              SourceMgr.getSpellingLineNumber(
----------------
ilya-biryukov wrote:
> 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.
offsetToPosition (if that's what you mean) uses a StringRef of the code, which is not handy in this case. I'll add another one "sourceLocToPosition" to convert a SourceLocation to a Position. WDYT? It can be used in a few other places too.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639