[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.cpp:103
+
+    if (SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin())) {
+      // Only inclusion directives in the main file make sense. The user cannot
----------------
NIT: replace `FilenameRange.getAsRange()` with `SR`


================
Comment at: clangd/ClangdUnit.cpp:126
+
+  CppFilePreambleCallbacks() : SourceMgr(nullptr) {}
+
----------------
NIT: remove ctor, use initializer on the member instead:
```
private:
  SourceManager *SourceMgr = nullptr;
```


================
Comment at: clangd/ClangdUnit.cpp:160
+  SourceManager *SourceMgr;
+  InclusionLocations IncLocations;
 };
----------------
Maybe swap `IncLocations` and `SourceMgr`?  Grouping `TopLevelDecls`, `TopLevelDeclIDs` and `IncLocations` together seems reasonable, as all of them are essentially output parameters.


================
Comment at: clangd/ClangdUnit.cpp:296
   StoreDiagsConsumer UnitDiagsConsumer(/*ref*/ ASTDiags);
+  InclusionLocations IncLocations;
+  // Copy over the includes from the preamble, then combine with the
----------------
NIT: move `IncLocations` closer to their first use, i.e. create the variable right before `addPPCallbacks()`


================
Comment at: clangd/ClangdUnit.h:51
 
+using IncludeReferenceMap = std::unordered_map<Range, Path, RangeHash>;
+
----------------
malaperle wrote:
> ilya-biryukov wrote:
> > 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.
> Done. I also renamed, IncludeReferenceMap  to InclusionLocations. I thought I was more suitable.
LG


================
Comment at: clangd/ClangdUnit.h:105
+  const InclusionLocations &getInclusionLocations() const {
+    return IncLocations;
+  };
----------------
NIT: move to .cpp file to be consisten with other decls in this file.


================
Comment at: clangd/XRefs.cpp:171
+    Range R = IncludeLoc.first;
+    const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
+    Position Pos = sourceLocToPosition(SourceMgr, SourceLocationBeg);
----------------
NIT: `SourceMgr` does not depend on the loop variable, declare it out of the loop.


================
Comment at: clangd/XRefs.cpp:174
+
+    if (R.start.line == Pos.line && R.start.character <= Pos.character &&
+        Pos.character <= R.end.character) {
----------------
NIT: define `operator <=` for `Position` and do this instead: `R.start <= Pos && Pos < R.end` (note that LSP ranges are half-open, i.e. the end is excluded).
Even better: define a `bool contains(Position Pos) const` helper in the `Range` class and use it here (will abstract away to half-open nature of the range)


================
Comment at: clangd/XRefs.cpp:178
+                                        Range{Position{0, 0}, Position{0, 0}}});
+      return IncludeTargets;
+    }
----------------
Let's follow the pattern of decls and macros here. I.e. not return from the function, merely collect all the results.


================
Comment at: unittests/clangd/XRefsTests.cpp:53
 
+class IgnoreDiagnostics : public DiagnosticsConsumer {
+  void onDiagnosticsReady(
----------------
NIT: remove this class, use `IgnoreDiagnostics` from `Compiler.h` instead.


================
Comment at: unittests/clangd/XRefsTests.cpp:245
+  const char *SourceContents = R"cpp(
+  #include "$2^invalid.h"
+  #include "^foo.h"
----------------
Could we also add tests for corner cases: cursor before opening quote, cursor after the closing quote, cursor in the middle of `#include` token? (we shouldn't navigate anywhere in the middle of the #include token)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38639