[PATCH] D153882: [clangd] Always allow diagnostics from stale preambles

[Kadir Cetinkaya via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
kadircet marked an inline comment as done.
Closed by commit rGbd89f9ec293e: [clangd] Always allow diagnostics from stale 
preambles (authored by kadircet).

Changed prior to commit:
  https://reviews.llvm.org/D153882?vs=534995=535287#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153882/new/

https://reviews.llvm.org/D153882

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
  clang-tools-extra/clangd/unittests/ModulesTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp

Index: clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
===
--- clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
+++ clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
@@ -402,7 +402,7 @@
 
   // The compiler should produce a diagnostic for hitting the
   // template instantiation depth.
-  ASSERT_TRUE(!AST.getDiagnostics()->empty());
+  ASSERT_FALSE(AST.getDiagnostics().empty());
 
   // Make sure getTypeHierarchy() doesn't get into an infinite recursion.
   // The parent is reported as "S" because "S<0>" is an invalid instantiation.
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -132,8 +132,6 @@
 llvm::errs() << "Failed to build code:\n" << Code;
 std::abort();
   }
-  assert(AST->getDiagnostics() &&
- "TestTU should always build an AST with a fresh Preamble");
   // Check for error diagnostics and report gtest failures (unless expected).
   // This guards against accidental syntax errors silently subverting tests.
   // error-ok is awfully primitive - using clang -verify would be nicer.
@@ -150,7 +148,7 @@
   }();
   if (!ErrorOk) {
 // We always build AST with a fresh preamble in TestTU.
-for (const auto  : *AST->getDiagnostics())
+for (const auto  : AST->getDiagnostics())
   if (D.Severity >= DiagnosticsEngine::Error) {
 llvm::errs()
 << "TestTU failed to build (suppress with /*error-ok*/): \n"
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -126,7 +126,7 @@
 class CaptureDiags : public ParsingCallbacks {
 public:
   void onMainAST(PathRef File, ParsedAST , PublishFn Publish) override {
-reportDiagnostics(File, *AST.getDiagnostics(), Publish);
+reportDiagnostics(File, AST.getDiagnostics(), Publish);
   }
 
   void onFailedAST(PathRef File, llvm::StringRef Version,
@@ -141,9 +141,8 @@
 if (!D)
   return;
 Publish([&]() {
-  const_cast<
-  llvm::unique_function)> &> (*D)(
-  File, std::move(Diags));
+  const_cast)> &>(
+  *D)(File, Diags);
 });
   }
 };
@@ -230,20 +229,24 @@
 Notification Ready;
 TUScheduler S(CDB, optsForTest(), captureDiags());
 auto Path = testPath("foo.cpp");
-updateWithDiags(S, Path, "", WantDiagnostics::Yes,
+// Semicolons here and in the following inputs are significant. They ensure
+// preamble stays the same across runs. Otherwise we might get multiple
+// diagnostics callbacks, once with the stale preamble and another with the
+// fresh preamble.
+updateWithDiags(S, Path, ";", WantDiagnostics::Yes,
 [&](std::vector) { Ready.wait(); });
-updateWithDiags(S, Path, "request diags", WantDiagnostics::Yes,
+updateWithDiags(S, Path, ";request diags", WantDiagnostics::Yes,
   

[PATCH] D153882: [clangd] Always allow diagnostics from stale preambles

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet marked an inline comment as done.
kadircet added inline comments.



Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:137
 Dict.handle("ClangTidy", [&](Node ) { parse(F.ClangTidy, N); });
-Dict.handle("AllowStalePreamble", [&](Node ) {
-  F.AllowStalePreamble = boolValue(N, "AllowStalePreamble");

hokein wrote:
> I wonder whether it is worth keeping this flag, but no-op (just emitting an 
> warning message, this flag is no-op).
> 
> Probably not worth, since this flag was introduced recently, and it is not in 
> any release.
that's what happens to unrecognized config keys already. user will get a diag 
message when parsing their config that says `AllowStalePreamble` is not 
recognized.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153882/new/

https://reviews.llvm.org/D153882

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

[PATCH] D153882: [clangd] Always allow diagnostics from stale preambles

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.



Comment at: clang-tools-extra/clangd/ConfigYAML.cpp:137
 Dict.handle("ClangTidy", [&](Node ) { parse(F.ClangTidy, N); });
-Dict.handle("AllowStalePreamble", [&](Node ) {
-  F.AllowStalePreamble = boolValue(N, "AllowStalePreamble");

I wonder whether it is worth keeping this flag, but no-op (just emitting an 
warning message, this flag is no-op).

Probably not worth, since this flag was introduced recently, and it is not in 
any release.



Comment at: clang-tools-extra/clangd/ParsedAST.h:157
   std::vector Marks;
   // Data, stored after parsing. std::nullopt if AST was built with a stale
   // preamble.

nit: update the comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153882/new/

https://reviews.llvm.org/D153882

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

[PATCH] D153882: [clangd] Always allow diagnostics from stale preambles

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet updated this revision to Diff 534995.
kadircet added a comment.

- Squash base commit


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153882/new/

https://reviews.llvm.org/D153882

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
  clang-tools-extra/clangd/unittests/ModulesTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp

Index: clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
===
--- clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
+++ clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
@@ -402,7 +402,7 @@
 
   // The compiler should produce a diagnostic for hitting the
   // template instantiation depth.
-  ASSERT_TRUE(!AST.getDiagnostics()->empty());
+  ASSERT_FALSE(AST.getDiagnostics().empty());
 
   // Make sure getTypeHierarchy() doesn't get into an infinite recursion.
   // The parent is reported as "S" because "S<0>" is an invalid instantiation.
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -132,8 +132,6 @@
 llvm::errs() << "Failed to build code:\n" << Code;
 std::abort();
   }
-  assert(AST->getDiagnostics() &&
- "TestTU should always build an AST with a fresh Preamble");
   // Check for error diagnostics and report gtest failures (unless expected).
   // This guards against accidental syntax errors silently subverting tests.
   // error-ok is awfully primitive - using clang -verify would be nicer.
@@ -150,7 +148,7 @@
   }();
   if (!ErrorOk) {
 // We always build AST with a fresh preamble in TestTU.
-for (const auto  : *AST->getDiagnostics())
+for (const auto  : AST->getDiagnostics())
   if (D.Severity >= DiagnosticsEngine::Error) {
 llvm::errs()
 << "TestTU failed to build (suppress with /*error-ok*/): \n"
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -126,7 +126,7 @@
 class CaptureDiags : public ParsingCallbacks {
 public:
   void onMainAST(PathRef File, ParsedAST , PublishFn Publish) override {
-reportDiagnostics(File, *AST.getDiagnostics(), Publish);
+reportDiagnostics(File, AST.getDiagnostics(), Publish);
   }
 
   void onFailedAST(PathRef File, llvm::StringRef Version,
@@ -141,9 +141,8 @@
 if (!D)
   return;
 Publish([&]() {
-  const_cast<
-  llvm::unique_function)> &> (*D)(
-  File, std::move(Diags));
+  const_cast)> &>(
+  *D)(File, Diags);
 });
   }
 };
@@ -230,20 +229,24 @@
 Notification Ready;
 TUScheduler S(CDB, optsForTest(), captureDiags());
 auto Path = testPath("foo.cpp");
-updateWithDiags(S, Path, "", WantDiagnostics::Yes,
+// Semicolons here and in the following inputs are significant. They ensure
+// preamble stays the same across runs. Otherwise we might get multiple
+// diagnostics callbacks, once with the stale preamble and another with the
+// fresh preamble.
+updateWithDiags(S, Path, ";", WantDiagnostics::Yes,
 [&](std::vector) { Ready.wait(); });
-updateWithDiags(S, Path, "request diags", WantDiagnostics::Yes,
+updateWithDiags(S, Path, ";request diags", WantDiagnostics::Yes,
 [&](std::vector) { ++CallbackCount; });
-updateWithDiags(S, Path, "auto (clobbered)", WantDiagnostics::Auto,
+updateWithDiags(S, Path, ";auto (clobbered)", WantDiagnostics::Auto,
 

[PATCH] D153882: [clangd] Always allow diagnostics from stale preambles

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet created this revision.
kadircet added a reviewer: hokein.
Herald added subscribers: arphaman, javed.absar.
Herald added a project: All.
kadircet requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

We've been running this internally for months now, without any
stability or correctness concerns. It has ~40% speed up on incremental
diagnostics latencies (as preamble can get invalidated through code completion
etc.).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D153882

Files:
  clang-tools-extra/clangd/ClangdLSPServer.cpp
  clang-tools-extra/clangd/ClangdLSPServer.h
  clang-tools-extra/clangd/ClangdServer.cpp
  clang-tools-extra/clangd/ClangdServer.h
  clang-tools-extra/clangd/Config.h
  clang-tools-extra/clangd/ConfigCompile.cpp
  clang-tools-extra/clangd/ConfigFragment.h
  clang-tools-extra/clangd/ConfigYAML.cpp
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/ParsedAST.h
  clang-tools-extra/clangd/Preamble.cpp
  clang-tools-extra/clangd/Preamble.h
  clang-tools-extra/clangd/TUScheduler.cpp
  clang-tools-extra/clangd/tool/Check.cpp
  clang-tools-extra/clangd/unittests/ClangdTests.cpp
  clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp
  clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp
  clang-tools-extra/clangd/unittests/ModulesTests.cpp
  clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
  clang-tools-extra/clangd/unittests/PreambleTests.cpp
  clang-tools-extra/clangd/unittests/SelectionTests.cpp
  clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp

Index: clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
===
--- clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
+++ clang-tools-extra/clangd/unittests/TypeHierarchyTests.cpp
@@ -402,7 +402,7 @@
 
   // The compiler should produce a diagnostic for hitting the
   // template instantiation depth.
-  ASSERT_TRUE(!AST.getDiagnostics()->empty());
+  ASSERT_FALSE(AST.getDiagnostics().empty());
 
   // Make sure getTypeHierarchy() doesn't get into an infinite recursion.
   // The parent is reported as "S" because "S<0>" is an invalid instantiation.
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -132,8 +132,6 @@
 llvm::errs() << "Failed to build code:\n" << Code;
 std::abort();
   }
-  assert(AST->getDiagnostics() &&
- "TestTU should always build an AST with a fresh Preamble");
   // Check for error diagnostics and report gtest failures (unless expected).
   // This guards against accidental syntax errors silently subverting tests.
   // error-ok is awfully primitive - using clang -verify would be nicer.
@@ -150,7 +148,7 @@
   }();
   if (!ErrorOk) {
 // We always build AST with a fresh preamble in TestTU.
-for (const auto  : *AST->getDiagnostics())
+for (const auto  : AST->getDiagnostics())
   if (D.Severity >= DiagnosticsEngine::Error) {
 llvm::errs()
 << "TestTU failed to build (suppress with /*error-ok*/): \n"
Index: clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
===
--- clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
+++ clang-tools-extra/clangd/unittests/TUSchedulerTests.cpp
@@ -126,7 +126,7 @@
 class CaptureDiags : public ParsingCallbacks {
 public:
   void onMainAST(PathRef File, ParsedAST , PublishFn Publish) override {
-reportDiagnostics(File, *AST.getDiagnostics(), Publish);
+reportDiagnostics(File, AST.getDiagnostics(), Publish);
   }
 
   void onFailedAST(PathRef File, llvm::StringRef Version,
@@ -141,9 +141,8 @@
 if (!D)
   return;
 Publish([&]() {
-  const_cast<
-  llvm::unique_function)> &> (*D)(
-  File, std::move(Diags));
+  const_cast)> &>(
+  *D)(File, Diags);
 });
   }
 };
@@ -230,20 +229,24 @@
 Notification Ready;
 TUScheduler S(CDB, optsForTest(), captureDiags());
 auto Path = testPath("foo.cpp");
-updateWithDiags(S, Path, "", WantDiagnostics::Yes,
+// Semicolons here and in the following inputs are significant. They ensure
+// preamble stays the same across runs. Otherwise we might get multiple
+// diagnostics callbacks, once with the stale preamble and another with the
+// fresh preamble.
+updateWithDiags(S, Path, ";", WantDiagnostics::Yes,
 [&](std::vector) { Ready.wait(); });
-