ilya-biryukov requested changes to this revision.
ilya-biryukov added a comment.
This revision now requires changes to proceed.
Thanks for addressing the comments quickly.
I took another look and added a few more comments.
This moves in the right direction, though, this is really close to landing.
================
Comment at: clangd/ClangdServer.cpp:25
#include <future>
+#include <unordered_map>
----------------
Is this include redundant now? Can we remove it?
================
Comment at: clangd/ClangdServer.cpp:430
+ Resources->getAST().get()->runUnderLock([Pos, &Result, &Ctx,
+ this](ParsedAST *AST) {
if (!AST)
----------------
Capturing `this` seems redundant. Remove it?
================
Comment at: clangd/ClangdUnit.cpp:85
+
+ if (SourceMgr.getMainFileID() ==
SourceMgr.getFileID(FilenameRange.getAsRange().getBegin()) &&
SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin())) {
+ // Only inclusion directives in the main file make sense. The user cannot
----------------
`clang-format` please
================
Comment at: clangd/ClangdUnit.cpp:85
+
+ if (SourceMgr.getMainFileID() ==
SourceMgr.getFileID(FilenameRange.getAsRange().getBegin()) &&
SourceMgr.isInMainFile(FilenameRange.getAsRange().getBegin())) {
+ // Only inclusion directives in the main file make sense. The user cannot
----------------
ilya-biryukov wrote:
> `clang-format` please
Do we need both checks? Doesn't `SourceMgr.isInMainFile` handles all the cases?
================
Comment at: clangd/ClangdUnit.cpp:113
+ CppFilePreambleCallbacks(IncludeReferenceMap &IRM)
+ : IRM(IRM) {}
----------------
Let's create a new empty map inside this class and have a
`takeIncludeReferences` method (similar to `TopLevelDeclIDs` and
`takeTopLevelDeclIDs`)
================
Comment at: clangd/ClangdUnit.cpp:147
+ IncludeReferenceMap &IRM;
+ std::vector<std::pair<SourceRange, std::string>>
TempPreambleIncludeLocations;
};
----------------
We should have `SourceMgr` at all the proper places now, let's store
`IncludeReferenceMap` directly
================
Comment at: clangd/ClangdUnit.cpp:281
+ IntrusiveRefCntPtr<vfs::FileSystem> VFS,
+ IncludeReferenceMap IRM) {
----------------
What reference does this `IncludeReferenceMap` contain? Includes from the
preamble?
================
Comment at: clangd/ClangdUnit.cpp:347
const SourceLocation &SearchedLocation,
- ASTContext &AST, Preprocessor &PP)
- : SearchedLocation(SearchedLocation), AST(AST), PP(PP) {}
+ ASTContext &AST, Preprocessor &PP, const
IncludeReferenceMap &IncludeLocationMap)
+ : SearchedLocation(SearchedLocation), AST(AST), PP(PP),
IncludeLocationMap(IncludeLocationMap) {}
----------------
This class handles processing AST and preprocessor, it does not need to get
`IncludeLocationMap` in constructor or store it at all.
Remove `IncludeLocationMap` from this class and handle getting references from
`IncludeLocationMap` outside this class instead.
================
Comment at: clangd/ClangdUnit.h:97
+ IncludeReferenceMap &getIRM() { return IRM; };
+
----------------
Make it all const?
`const IncludeReferenceMap &getIRM() const { return IRM; }`
================
Comment at: clangd/ClangdUnit.h:276
+std::vector<Location>
+findDefinitions(const Context &Ctx, ParsedAST &AST, Position Pos);
+
----------------
This function moved without need and lost a comment.
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
