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