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