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

[Ilya Biryukov via Phabricator via cfe-commits]

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