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:
  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.

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 

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, 
+      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)

  rCTE Clang Tools Extra

cfe-commits mailing list

Reply via email to