ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdUnit.h:51
+using IncludeReferenceMap = std::unordered_map<Range, Path, RangeHash>;
+
----------------
We use `unordered_map` as a `vector<pair<>>` here. (i.e. we never look up
values by key, because we don't only the range, merely a point in the range)
Replace map with `vector<pair<>>` and remove `RangeHash` that we don't need
anymore.
================
Comment at: clangd/XRefs.cpp:185
+
+ if ((unsigned)R.start.line ==
+ SourceMgr.getSpellingLineNumber(
----------------
malaperle wrote:
> 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.
Looks good, thanks!
================
Comment at: unittests/clangd/ClangdTests.cpp:781
+
+ Position P = Position{1, 11};
+
----------------
We moved `findDefinition` tests to `XRefsTests.cpp` and added a useful
`Annotation` helper to make writing these tests simpler.
Maybe we could use it for this test as well?
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
