ilya-biryukov updated this revision to Diff 110155.
ilya-biryukov added a comment.
Addressed review comments.
- Added a test for forceReparse.
- Got rid of a redundant `= nullptr` assignment.
https://reviews.llvm.org/D36398
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnitStore.cpp
clangd/ClangdUnitStore.h
unittests/clangd/ClangdTests.cpp
Index: unittests/clangd/ClangdTests.cpp
===================================================================
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -500,6 +500,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: clangd/ClangdUnitStore.h
===================================================================
--- clangd/ClangdUnitStore.h
+++ 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: clangd/ClangdUnitStore.cpp
===================================================================
--- clangd/ClangdUnitStore.cpp
+++ 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());
+}
Index: clangd/ClangdServer.h
===================================================================
--- clangd/ClangdServer.h
+++ 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: clangd/ClangdServer.cpp
===================================================================
--- clangd/ClangdServer.cpp
+++ 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>>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
