[PATCH] D38639: [clangd] #include statements support for Open definition
Ilya Biryukov via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 18 02:22:40 PST 2017
ilya-biryukov requested changes to this revision.
ilya-biryukov added inline comments.
This revision now requires changes to proceed.
================
Comment at: clangd/ClangdServer.cpp:446
std::vector<Location> Result;
- Resources->getAST().get()->runUnderLock([Pos, &Result, this](ParsedAST *AST) {
+ std::shared_future<std::shared_ptr<const PreambleData>> Preamble =
+ Resources->getPreamble();
----------------
We don't need this `Preamble` variable, right?
================
Comment at: clangd/ClangdUnit.cpp:73
-class CppFilePreambleCallbacks : public PreambleCallbacks {
+class CppFilePreambleCallbacks : public PreambleCallbacks, public PPCallbacks {
public:
----------------
We don't need to derive this class from `PPCallbacks`, `DelegatingPPCallbacks` can call any method on it anyway as it knows the exact type. Please remove this inheritance.
================
Comment at: clangd/ClangdUnit.cpp:79
+ CppFilePreambleCallbacks(SourceManager *SourceMgr, IncludeReferenceMap &IRM)
+ : SourceMgr(SourceMgr), IRM(IRM) {}
----------------
Instead of passing `SourceManager` in constructor, maybe add a `BeforeExecute(CompilerInstance &CI)` method to `PreambleCallbacks` and set `SourceManager` when its called.
================
Comment at: clangd/ClangdUnit.cpp:167
+ llvm::make_unique<DelegatingPPCallbacks>(*this);
+ return DelegatedPPCallbacks;
+ }
----------------
NIT: replace with `return llvm::make_unique<DelegatingPPCallbacks>(*this);` and remove the local variable.
================
Comment at: clangd/ClangdUnit.cpp:175
+ IncludeReferenceMap &IRM;
+ std::vector<std::pair<SourceRange, std::string>> TempPreambleIncludeLocations;
};
----------------
Adding `BeforeExecute` would allow to get rid of this vector.
================
Comment at: clangd/ClangdUnit.cpp:247
IntrusiveRefCntPtr<vfs::FileSystem> VFS,
- clangd::Logger &Logger) {
+ clangd::Logger &Logger, IncludeReferenceMap &IRM) {
----------------
Let's store `IncludeReferenceMap` in `ParsedAST` instead of having it as out parameter.
It allows to properly manage the lifetime of `IncludeReferenceMap` (logically, it is exactly tied to the AST).
================
Comment at: clangd/ClangdUnit.cpp:270
+
+ CppFilePreambleCallbacks SerializedDeclsCollector(&Clang->getSourceManager(),
+ IRM);
----------------
Why do we use `CppFilePreambleCallbacks` here? Factor the code that handles the references into a separate class and use it here:
```
class IncludeRefsCollector : public PPCallbacks {
public:
IncludeRefsCollector(IncludeReferenceMap &Refs) : Refs(Refs) {}
// implementation of PPCallbacks ...
private;
IncludeReferenceMap &Refs;
};
/// .....
class CppFilePreambleCaallbacks {
public:
// ....
std::unique_ptr<PPCallbacks> createPPCallbacks() { return llvm::make_unique<IncludeRefsCollector>(this->IRM); }
};
// ....
void ParsedAST::Build() {
// ....
// No need to create CppFilePreambleCallbacks here.
Clang->getPreprocessor().addPPCallbacks(llvm::make_unique<IncludeRefsCollector>(IRM));
//....
}
```
================
Comment at: clangd/ClangdUnit.cpp:309
Preprocessor &PP;
+ std::unordered_map<Range, Path, RangeHash> IncludeLocationMap;
----------------
Use `IncludeReferenceMap` typedef here.
================
Comment at: clangd/ClangdUnit.cpp:315
+ Preprocessor &PP,
+ std::unordered_map<Range, Path, RangeHash> IncludeLocationMap)
+ : SearchedLocation(SearchedLocation), AST(AST), PP(PP),
----------------
- use a typedef for the map here
- accept the map by const reference to avoid copies
================
Comment at: clangd/ClangdUnit.cpp:365
Range R = {Begin, End};
+ addLocation(URI::fromFile(
+ SourceMgr.getFilename(SourceMgr.getSpellingLoc(LocStart))),
----------------
Do we change semantics of getting the file paths here? Maybe leave the code as is?
================
Comment at: clangd/ClangdUnit.cpp:370
+
+ void addLocation(URI Uri, Range R) {
Location L;
----------------
This method does not seem particularly useful. One can construct location like this: `Location{Uri, R}`, the whole method can be written as `DeclarationLocations.push_back(Location{Uri, R})`.
================
Comment at: clangd/ClangdUnit.cpp:378
void finish() override {
+
+ for (auto &IncludeLoc : IncludeLocationMap) {
----------------
NIT: empty linke at the start of the function
================
Comment at: clangd/ClangdUnit.cpp:381
+ Range R = IncludeLoc.first;
+ if (isSameLine(R.start.line)) {
+ addLocation(URI::fromFile(IncludeLoc.second), Range{Position{0,0}, Position{0,0}});
----------------
Let's check the the point is in range, not that it's not the same line. So that we don't trigger goto in strange places, like comments:
```
#include <vector> // this include is very important. <--- we should not skip to include when editor caret is over a comment
```
================
Comment at: clangd/ClangdUnit.cpp:420
+ ParsedAST &AST, Position Pos, clangd::Logger &Logger,
+ std::unordered_map<Range, Path, RangeHash> IncludeLocationMap) {
const SourceManager &SourceMgr = AST.getASTContext().getSourceManager();
----------------
If we store `IncludeLocationMap` in `ParsedAST` we don't need to change interface of this function.
================
Comment at: clangd/ClangdUnit.h:63
+using IncludeReferenceMap = std::unordered_map<Range, Path, RangeHash>;
+
----------------
We store a map, but only iterate through it in order. Let's use `vector<pair<Range, Path>>` instead.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D38639
More information about the cfe-commits
mailing list