This revision was automatically updated to reflect the committed changes. Closed by commit rL310819: [clangd] Check if CompileCommand has changed on forceReparse. (authored by ibiryukov).
Repository: rL LLVM https://reviews.llvm.org/D36398 Files: clang-tools-extra/trunk/clangd/ClangdServer.cpp clang-tools-extra/trunk/clangd/ClangdServer.h clang-tools-extra/trunk/clangd/ClangdUnitStore.cpp clang-tools-extra/trunk/clangd/ClangdUnitStore.h clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp
Index: clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp =================================================================== --- clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp +++ clang-tools-extra/trunk/unittests/clangd/ClangdTests.cpp @@ -501,6 +501,53 @@ } #endif // LLVM_ON_UNIX +TEST_F(ClangdVFSTest, ForceReparseCompileCommand) { + MockFSProvider FS; + ErrorCheckingDiagConsumer DiagConsumer; + MockCompilationDatabase CDB(/*AddFreestandingFlag=*/true); + ClangdServer Server(CDB, DiagConsumer, FS, + /*RunSynchronously=*/true); + // No need to sync reparses, because RunSynchronously is set + // to true. + + auto FooCpp = getVirtualTestFilePath("foo.cpp"); + const auto SourceContents1 = R"cpp( +template <class T> +struct foo { T x; }; +)cpp"; + const auto SourceContents2 = R"cpp( +template <class T> +struct bar { T x; }; +)cpp"; + + FS.Files[FooCpp] = ""; + FS.ExpectedFile = FooCpp; + + // First parse files in C mode and check they produce errors. + CDB.ExtraClangFlags = {"-xc"}; + Server.addDocument(FooCpp, SourceContents1); + EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); + Server.addDocument(FooCpp, SourceContents2); + EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); + + // Now switch to C++ mode. + CDB.ExtraClangFlags = {"-xc++"}; + // Currently, addDocument never checks if CompileCommand has changed, so we + // expect to see the errors. + Server.addDocument(FooCpp, SourceContents1); + EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); + Server.addDocument(FooCpp, SourceContents2); + EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags()); + // But forceReparse should reparse the file with proper flags. + Server.forceReparse(FooCpp); + EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); + // Subsequent addDocument calls should finish without errors too. + Server.addDocument(FooCpp, SourceContents1); + EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); + Server.addDocument(FooCpp, SourceContents2); + EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags()); +} + class ClangdCompletionTest : public ClangdVFSTest { protected: bool ContainsItem(std::vector<CompletionItem> const &Items, StringRef Name) { Index: clang-tools-extra/trunk/clangd/ClangdServer.h =================================================================== --- clang-tools-extra/trunk/clangd/ClangdServer.h +++ clang-tools-extra/trunk/clangd/ClangdServer.h @@ -192,10 +192,13 @@ std::future<void> addDocument(PathRef File, StringRef Contents); /// Remove \p File from list of tracked files, schedule a request to free /// resources associated with it. - /// \return A future that will become ready the file is removed and all - /// associated reosources are freed. + /// \return A future that will become ready when the file is removed and all + /// associated resources are freed. std::future<void> removeDocument(PathRef File); /// Force \p File to be reparsed using the latest contents. + /// Will also check if CompileCommand, provided by GlobalCompilationDatabase + /// for \p File has changed. If it has, will remove currently stored Preamble + /// and AST and rebuild them from scratch. std::future<void> forceReparse(PathRef File); /// Run code completion for \p File at \p Pos. If \p OverridenContents is not Index: clang-tools-extra/trunk/clangd/ClangdServer.cpp =================================================================== --- clang-tools-extra/trunk/clangd/ClangdServer.cpp +++ clang-tools-extra/trunk/clangd/ClangdServer.cpp @@ -154,9 +154,20 @@ } std::future<void> ClangdServer::forceReparse(PathRef File) { - // The addDocument schedules the reparse even if the contents of the file - // never changed, so we just call it here. - return addDocument(File, getDocument(File)); + auto FileContents = DraftMgr.getDraft(File); + assert(FileContents.Draft && + "forceReparse() was called for non-added document"); + + auto TaggedFS = FSProvider.getTaggedFileSystem(File); + auto Recreated = Units.recreateFileIfCompileCommandChanged( + File, ResourceDir, CDB, PCHs, TaggedFS.Value); + + // Note that std::future from this cleanup action is ignored. + scheduleCancelRebuild(std::move(Recreated.RemovedFile)); + // Schedule a reparse. + return scheduleReparseAndDiags(File, std::move(FileContents), + std::move(Recreated.FileInCollection), + std::move(TaggedFS)); } Tagged<std::vector<CompletionItem>> Index: clang-tools-extra/trunk/clangd/ClangdUnitStore.h =================================================================== --- clang-tools-extra/trunk/clangd/ClangdUnitStore.h +++ clang-tools-extra/trunk/clangd/ClangdUnitStore.h @@ -42,6 +42,25 @@ return It->second; } + struct RecreateResult { + /// A CppFile, stored in this CppFileCollection for the corresponding + /// filepath after calling recreateFileIfCompileCommandChanged. + std::shared_ptr<CppFile> FileInCollection; + /// If a new CppFile had to be created to account for changed + /// CompileCommand, a previous CppFile instance will be returned in this + /// field. + std::shared_ptr<CppFile> RemovedFile; + }; + + /// Similar to getOrCreateFile, but will replace a current CppFile for \p File + /// with a new one if CompileCommand, provided by \p CDB has changed. + /// If a currently stored CppFile had to be replaced, the previous instance + /// will be returned in RecreateResult.RemovedFile. + RecreateResult recreateFileIfCompileCommandChanged( + PathRef File, PathRef ResourceDir, GlobalCompilationDatabase &CDB, + std::shared_ptr<PCHContainerOperations> PCHs, + IntrusiveRefCntPtr<vfs::FileSystem> VFS); + std::shared_ptr<CppFile> getFile(PathRef File) { std::lock_guard<std::mutex> Lock(Mutex); @@ -59,6 +78,9 @@ tooling::CompileCommand getCompileCommand(GlobalCompilationDatabase &CDB, PathRef File, PathRef ResourceDir); + bool compileCommandsAreEqual(tooling::CompileCommand const &LHS, + tooling::CompileCommand const &RHS); + std::mutex Mutex; llvm::StringMap<std::shared_ptr<CppFile>> OpenedFiles; }; Index: clang-tools-extra/trunk/clangd/ClangdUnitStore.cpp =================================================================== --- clang-tools-extra/trunk/clangd/ClangdUnitStore.cpp +++ clang-tools-extra/trunk/clangd/ClangdUnitStore.cpp @@ -9,6 +9,7 @@ #include "ClangdUnitStore.h" #include "llvm/Support/Path.h" +#include <algorithm> using namespace clang::clangd; using namespace clang; @@ -25,6 +26,32 @@ return Result; } +CppFileCollection::RecreateResult +CppFileCollection::recreateFileIfCompileCommandChanged( + PathRef File, PathRef ResourceDir, GlobalCompilationDatabase &CDB, + std::shared_ptr<PCHContainerOperations> PCHs, + IntrusiveRefCntPtr<vfs::FileSystem> VFS) { + auto NewCommand = getCompileCommand(CDB, File, ResourceDir); + + std::lock_guard<std::mutex> Lock(Mutex); + + RecreateResult Result; + + auto It = OpenedFiles.find(File); + if (It == OpenedFiles.end()) { + It = OpenedFiles + .try_emplace(File, CppFile::Create(File, std::move(NewCommand), + std::move(PCHs))) + .first; + } else if (!compileCommandsAreEqual(It->second->getCompileCommand(), + NewCommand)) { + Result.RemovedFile = std::move(It->second); + It->second = CppFile::Create(File, std::move(NewCommand), std::move(PCHs)); + } + Result.FileInCollection = It->second; + return Result; +} + tooling::CompileCommand CppFileCollection::getCompileCommand(GlobalCompilationDatabase &CDB, PathRef File, PathRef ResourceDir) { @@ -39,3 +66,12 @@ std::string(ResourceDir)); return std::move(Commands.front()); } + +bool CppFileCollection::compileCommandsAreEqual( + tooling::CompileCommand const &LHS, tooling::CompileCommand const &RHS) { + // tooling::CompileCommand.Output is ignored, it's not relevant for clangd. + return LHS.Directory == RHS.Directory && + LHS.CommandLine.size() == RHS.CommandLine.size() && + std::equal(LHS.CommandLine.begin(), LHS.CommandLine.end(), + RHS.CommandLine.begin()); +}
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits