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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
