[PATCH] D43454: [clangd] Do not reuse preamble if compile args changed

[Phabricator via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL325522: [clangd] Do not reuse preamble if compile args 
changed (authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D43454?vs=134878&id=134941#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43454

Files:
  clang-tools-extra/trunk/clangd/ClangdUnit.cpp
  clang-tools-extra/trunk/clangd/ClangdUnit.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
@@ -373,6 +373,46 @@
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 }
 
+TEST_F(ClangdVFSTest, ForceReparseCompileCommandDefines) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, DiagConsumer, FS,
+  /*AsyncThreadsCount=*/0,
+  /*StorePreamblesInMemory=*/true);
+
+  // No need to sync reparses, because reparses are performed on the calling
+  // thread.
+  auto FooCpp = testPath("foo.cpp");
+  const auto SourceContents = R"cpp(
+#ifdef WITH_ERROR
+this
+#endif
+
+int main() { return 0; }
+)cpp";
+  FS.Files[FooCpp] = "";
+  FS.ExpectedFile = FooCpp;
+
+  // Parse with define, we expect to see the errors.
+  CDB.ExtraClangFlags = {"-DWITH_ERROR"};
+  Server.addDocument(FooCpp, SourceContents);
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+
+  // Parse without the define, no errors should be produced.
+  CDB.ExtraClangFlags = {};
+  // Currently, addDocument never checks if CompileCommand has changed, so we
+  // expect to see the errors.
+  Server.addDocument(FooCpp, SourceContents);
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+  // But forceReparse should reparse the file with proper flags.
+  Server.forceReparse(FooCpp);
+  EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
+  // Subsequent addDocument call should finish without errors too.
+  Server.addDocument(FooCpp, SourceContents);
+  EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
+}
+
 TEST_F(ClangdVFSTest, MemoryUsage) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
Index: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp
@@ -34,6 +34,13 @@
 
 namespace {
 
+bool compileCommandsAreEqual(const tooling::CompileCommand &LHS,
+ const tooling::CompileCommand &RHS) {
+  // We don't check for Output, it should not matter to clangd.
+  return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
+ llvm::makeArrayRef(LHS.CommandLine).equals(RHS.CommandLine);
+}
+
 template  std::size_t getUsedBytes(const std::vector &Vec) {
   return Vec.capacity() * sizeof(T);
 }
@@ -417,7 +424,7 @@
 
   // Compute updated Preamble.
   std::shared_ptr NewPreamble =
-  rebuildPreamble(*CI, Inputs.FS, *ContentsBuffer);
+  rebuildPreamble(*CI, Inputs.CompileCommand, Inputs.FS, *ContentsBuffer);
 
   // Remove current AST to avoid wasting memory.
   AST = llvm::None;
@@ -445,6 +452,7 @@
   }
 
   // Write the results of rebuild into class fields.
+  Command = std::move(Inputs.CompileCommand);
   Preamble = std::move(NewPreamble);
   AST = std::move(NewAST);
   return Diagnostics;
@@ -471,11 +479,12 @@
 
 std::shared_ptr
 CppFile::rebuildPreamble(CompilerInvocation &CI,
+ const tooling::CompileCommand &Command,
  IntrusiveRefCntPtr FS,
  llvm::MemoryBuffer &ContentsBuffer) const {
   const auto &OldPreamble = this->Preamble;
   auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), &ContentsBuffer, 0);
-  if (OldPreamble &&
+  if (OldPreamble && compileCommandsAreEqual(this->Command, Command) &&
   OldPreamble->Preamble.CanReuse(CI, &ContentsBuffer, Bounds, FS.get())) {
 log("Reusing preamble for file " + Twine(FileName));
 return OldPreamble;
Index: clang-tools-extra/trunk/clangd/ClangdUnit.h
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.h
+++ clang-tools-extra/trunk/clangd/ClangdUnit.h
@@ -152,12 +152,15 @@
   /// This method is const to ensure we don't incidentally modify any fields.
   std::shared_ptr
   rebuildPreamble(CompilerInvocation &CI,
+  const tooling::CompileCommand &Command,
   IntrusiveRefCntPtr FS,
   llvm::MemoryBuffer &ContentsBuffer) const;
 
   const Path FileName;
   const bool StorePreamblesInMemory;
 
+  /// The last CompileCommand used to build AST and Preamble.
+  tooling::CompileCommand Command;
   /// 

[PATCH] D43454: [clangd] Do not reuse preamble if compile args changed

[Simon Marchi via Phabricator via cfe-commits]

simark accepted this revision.
simark added a comment.
This revision is now accepted and ready to land.

I don't have the setup to test at the moment, but the code looks good to me.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43454



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D43454: [clangd] Do not reuse preamble if compile args changed

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov created this revision.
ilya-biryukov added reviewers: hokein, ioeric, sammccall.
Herald added subscribers: cfe-commits, jkorous-apple, klimek.

Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43454

Files:
  clangd/ClangdUnit.cpp
  clangd/ClangdUnit.h
  unittests/clangd/ClangdTests.cpp

Index: unittests/clangd/ClangdTests.cpp
===
--- unittests/clangd/ClangdTests.cpp
+++ unittests/clangd/ClangdTests.cpp
@@ -373,6 +373,46 @@
   EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
 }
 
+TEST_F(ClangdVFSTest, ForceReparseCompileCommandDefines) {
+  MockFSProvider FS;
+  ErrorCheckingDiagConsumer DiagConsumer;
+  MockCompilationDatabase CDB;
+  ClangdServer Server(CDB, DiagConsumer, FS,
+  /*AsyncThreadsCount=*/0,
+  /*StorePreamblesInMemory=*/true);
+
+  // No need to sync reparses, because reparses are performed on the calling
+  // thread.
+  auto FooCpp = testPath("foo.cpp");
+  const auto SourceContents = R"cpp(
+#ifdef WITH_ERROR
+this
+#endif
+
+int main() { return 0; }
+)cpp";
+  FS.Files[FooCpp] = "";
+  FS.ExpectedFile = FooCpp;
+
+  // Parse with define, we expect to see the errors.
+  CDB.ExtraClangFlags = {"-DWITH_ERROR"};
+  Server.addDocument(FooCpp, SourceContents);
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+
+  // Parse without the define, no errors should be produced.
+  CDB.ExtraClangFlags = {};
+  // Currently, addDocument never checks if CompileCommand has changed, so we
+  // expect to see the errors.
+  Server.addDocument(FooCpp, SourceContents);
+  EXPECT_TRUE(DiagConsumer.hadErrorInLastDiags());
+  // But forceReparse should reparse the file with proper flags.
+  Server.forceReparse(FooCpp);
+  EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
+  // Subsequent addDocument call should finish without errors too.
+  Server.addDocument(FooCpp, SourceContents);
+  EXPECT_FALSE(DiagConsumer.hadErrorInLastDiags());
+}
+
 TEST_F(ClangdVFSTest, MemoryUsage) {
   MockFSProvider FS;
   ErrorCheckingDiagConsumer DiagConsumer;
Index: clangd/ClangdUnit.h
===
--- clangd/ClangdUnit.h
+++ clangd/ClangdUnit.h
@@ -152,12 +152,15 @@
   /// This method is const to ensure we don't incidentally modify any fields.
   std::shared_ptr
   rebuildPreamble(CompilerInvocation &CI,
+  const tooling::CompileCommand &Command,
   IntrusiveRefCntPtr FS,
   llvm::MemoryBuffer &ContentsBuffer) const;
 
   Path FileName;
   bool StorePreamblesInMemory;
 
+  /// The last CompileCommand used to build AST and Preamble.
+  tooling::CompileCommand Command;
   /// The last parsed AST.
   llvm::Optional AST;
   /// The last built Preamble.
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -34,6 +34,13 @@
 
 namespace {
 
+bool compileCommandsAreEqual(const tooling::CompileCommand &LHS,
+ const tooling::CompileCommand &RHS) {
+  // We don't check for Output, it should not matter to clangd.
+  return LHS.Directory == RHS.Directory && LHS.Filename == RHS.Filename &&
+ llvm::makeArrayRef(LHS.CommandLine).equals(RHS.CommandLine);
+}
+
 template  std::size_t getUsedBytes(const std::vector &Vec) {
   return Vec.capacity() * sizeof(T);
 }
@@ -417,7 +424,7 @@
 
   // Compute updated Preamble.
   std::shared_ptr NewPreamble =
-  rebuildPreamble(*CI, Inputs.FS, *ContentsBuffer);
+  rebuildPreamble(*CI, Inputs.CompileCommand, Inputs.FS, *ContentsBuffer);
 
   // Remove current AST to avoid wasting memory.
   AST = llvm::None;
@@ -445,6 +452,7 @@
   }
 
   // Write the results of rebuild into class fields.
+  Command = std::move(Inputs.CompileCommand);
   Preamble = std::move(NewPreamble);
   AST = std::move(NewAST);
   return Diagnostics;
@@ -471,11 +479,12 @@
 
 std::shared_ptr
 CppFile::rebuildPreamble(CompilerInvocation &CI,
+ const tooling::CompileCommand &Command,
  IntrusiveRefCntPtr FS,
  llvm::MemoryBuffer &ContentsBuffer) const {
   const auto &OldPreamble = this->Preamble;
   auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), &ContentsBuffer, 0);
-  if (OldPreamble &&
+  if (OldPreamble && compileCommandsAreEqual(this->Command, Command) &&
   OldPreamble->Preamble.CanReuse(CI, &ContentsBuffer, Bounds, FS.get())) {
 log("Reusing preamble for file " + Twine(FileName));
 return OldPreamble;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits