[PATCH] D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy

[Fangrui Song via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL361113: [clangd] Respect WarningsAsErrors configuration for 
clang-tidy (authored by MaskRay, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D61841?vs=199956=200168#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D61841

Files:
  clang-tools-extra/trunk/clangd/ClangdUnit.cpp
  clang-tools-extra/trunk/clangd/Diagnostics.cpp
  clang-tools-extra/trunk/clangd/Diagnostics.h
  clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/trunk/clangd/unittests/TestTU.cpp
  clang-tools-extra/trunk/clangd/unittests/TestTU.h

Index: clang-tools-extra/trunk/clangd/unittests/TestTU.h
===
--- clang-tools-extra/trunk/clangd/unittests/TestTU.h
+++ clang-tools-extra/trunk/clangd/unittests/TestTU.h
@@ -57,6 +57,7 @@
   std::vector ExtraArgs;
 
   llvm::Optional ClangTidyChecks;
+  llvm::Optional ClangTidyWarningsAsErrors;
   // Index to use when building AST.
   const SymbolIndex *ExternalIndex = nullptr;
 
Index: clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp
@@ -73,6 +73,7 @@
 
 MATCHER_P(DiagSource, S, "") { return arg.Source == S; }
 MATCHER_P(DiagName, N, "") { return arg.Name == N; }
+MATCHER_P(DiagSeverity, S, "") { return arg.Severity == S; }
 
 MATCHER_P(EqualToFix, Fix, "LSP fix " + llvm::to_string(Fix)) {
   if (arg.Message != Fix.Message)
@@ -227,6 +228,44 @@
   DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division";
 }
 
+TEST(DiagnosticTest, ClangTidyWarningAsError) {
+  Annotations Main(R"cpp(
+int main() {
+  int i = 3;
+  double f = [[8]] / i;
+}
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "bugprone-integer-division";
+  TU.ClangTidyWarningsAsErrors = "bugprone-integer-division";
+  EXPECT_THAT(
+  TU.build().getDiagnostics(),
+  UnorderedElementsAre(::testing::AllOf(
+  Diag(Main.range(), "result of integer division used in a floating "
+ "point context; possible loss of precision"),
+  DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"),
+  DiagSeverity(DiagnosticsEngine::Error;
+}
+
+TEST(DiagnosticTest, ClangTidyWarningAsErrorTrumpsSuppressionComment) {
+  Annotations Main(R"cpp(
+int main() {
+  int i = 3;
+  double f = [[8]] / i;  // NOLINT
+}
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "bugprone-integer-division";
+  TU.ClangTidyWarningsAsErrors = "bugprone-integer-division";
+  EXPECT_THAT(
+  TU.build().getDiagnostics(),
+  UnorderedElementsAre(::testing::AllOf(
+  Diag(Main.range(), "result of integer division used in a floating "
+ "point context; possible loss of precision"),
+  DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"),
+  DiagSeverity(DiagnosticsEngine::Error;
+}
+
 TEST(DiagnosticsTest, Preprocessor) {
   // This looks like a preamble, but there's an #else in the middle!
   // Check that:
Index: clang-tools-extra/trunk/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/trunk/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/trunk/clangd/unittests/TestTU.cpp
@@ -48,6 +48,7 @@
   Inputs.FS = buildTestFS(Files);
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
+  Inputs.Opts.ClangTidyOpts.WarningsAsErrors = ClangTidyWarningsAsErrors;
   Inputs.Index = ExternalIndex;
   if (Inputs.Index)
 Inputs.Opts.SuggestMissingIncludes = true;
Index: clang-tools-extra/trunk/clangd/Diagnostics.h
===
--- clang-tools-extra/trunk/clangd/Diagnostics.h
+++ clang-tools-extra/trunk/clangd/Diagnostics.h
@@ -128,20 +128,20 @@
 
   using DiagFixer = std::function(DiagnosticsEngine::Level,
const clang::Diagnostic &)>;
-  using DiagFilter =
-  std::function;
+  using LevelAdjuster = std::function;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.
   void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
-  /// If set, ignore diagnostics for which \p SuppressionFilter returns true.
-  void suppressDiagnostics(DiagFilter SuppressionFilter) {
-this->SuppressionFilter = SuppressionFilter;
-  }
+  /// If set, this allows the client of this class to adjust the level of
+  /// diagnostics, such as promoting warnings to errors, or 

[PATCH] D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy

[Sam McCall via Phabricator via cfe-commits]

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

LG, thanks for the changes!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61841



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

[PATCH] D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy

[Nathan Ridge via Phabricator via cfe-commits]

nridge edited reviewers, added: sammccall; removed: ilya-biryukov.
nridge added a subscriber: sammccall.
nridge added a comment.
Herald added a subscriber: ilya-biryukov.

Changing reviewer to @sammccall as the updates to the patch are closely related 
to the discussion in D60953 .

Also added a couple of test cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61841



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

[PATCH] D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 199956.
nridge added a comment.

Update as per changes to dependent patch D60953 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61841

Files:
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/Diagnostics.cpp
  clang-tools-extra/clangd/Diagnostics.h
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
  clang-tools-extra/clangd/unittests/TestTU.cpp
  clang-tools-extra/clangd/unittests/TestTU.h

Index: clang-tools-extra/clangd/unittests/TestTU.h
===
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -57,6 +57,7 @@
   std::vector ExtraArgs;
 
   llvm::Optional ClangTidyChecks;
+  llvm::Optional ClangTidyWarningsAsErrors;
   // Index to use when building AST.
   const SymbolIndex *ExternalIndex = nullptr;
 
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -48,6 +48,7 @@
   Inputs.FS = buildTestFS(Files);
   Inputs.Opts = ParseOptions();
   Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks;
+  Inputs.Opts.ClangTidyOpts.WarningsAsErrors = ClangTidyWarningsAsErrors;
   Inputs.Index = ExternalIndex;
   if (Inputs.Index)
 Inputs.Opts.SuggestMissingIncludes = true;
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -73,6 +73,7 @@
 
 MATCHER_P(DiagSource, S, "") { return arg.Source == S; }
 MATCHER_P(DiagName, N, "") { return arg.Name == N; }
+MATCHER_P(DiagSeverity, S, "") { return arg.Severity == S; }
 
 MATCHER_P(EqualToFix, Fix, "LSP fix " + llvm::to_string(Fix)) {
   if (arg.Message != Fix.Message)
@@ -227,6 +228,44 @@
   DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division";
 }
 
+TEST(DiagnosticTest, ClangTidyWarningAsError) {
+  Annotations Main(R"cpp(
+int main() {
+  int i = 3;
+  double f = [[8]] / i;
+}
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "bugprone-integer-division";
+  TU.ClangTidyWarningsAsErrors = "bugprone-integer-division";
+  EXPECT_THAT(
+  TU.build().getDiagnostics(),
+  UnorderedElementsAre(::testing::AllOf(
+  Diag(Main.range(), "result of integer division used in a floating "
+ "point context; possible loss of precision"),
+  DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"),
+  DiagSeverity(DiagnosticsEngine::Error;
+}
+
+TEST(DiagnosticTest, ClangTidyWarningAsErrorTrumpsSuppressionComment) {
+  Annotations Main(R"cpp(
+int main() {
+  int i = 3;
+  double f = [[8]] / i;  // NOLINT
+}
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "bugprone-integer-division";
+  TU.ClangTidyWarningsAsErrors = "bugprone-integer-division";
+  EXPECT_THAT(
+  TU.build().getDiagnostics(),
+  UnorderedElementsAre(::testing::AllOf(
+  Diag(Main.range(), "result of integer division used in a floating "
+ "point context; possible loss of precision"),
+  DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"),
+  DiagSeverity(DiagnosticsEngine::Error;
+}
+
 TEST(DiagnosticsTest, Preprocessor) {
   // This looks like a preamble, but there's an #else in the middle!
   // Check that:
Index: clang-tools-extra/clangd/Diagnostics.h
===
--- clang-tools-extra/clangd/Diagnostics.h
+++ clang-tools-extra/clangd/Diagnostics.h
@@ -128,20 +128,20 @@
 
   using DiagFixer = std::function(DiagnosticsEngine::Level,
const clang::Diagnostic &)>;
-  using DiagFilter =
-  std::function;
+  using LevelAdjuster = std::function;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.
   void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
-  /// If set, ignore diagnostics for which \p SuppressionFilter returns true.
-  void suppressDiagnostics(DiagFilter SuppressionFilter) {
-this->SuppressionFilter = SuppressionFilter;
-  }
+  /// If set, this allows the client of this class to adjust the level of
+  /// diagnostics, such as promoting warnings to errors, or ignoring
+  /// diagnostics.
+  void setLevelAdjuster(LevelAdjuster Adjuster) { this->Adjuster = Adjuster; }
 
 private:
   void flushLastDiag();
 
   DiagFixer Fixer = nullptr;
-  DiagFilter SuppressionFilter = nullptr;
+  LevelAdjuster Adjuster = nullptr;
   std::vector Output;
   llvm::Optional 

[PATCH] D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy

[Nathan Ridge via Phabricator via cfe-commits]

nridge added inline comments.



Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:346
+  std::string CheckName = CTContext->getCheckName(Info.getID());
+  if (!CheckName.empty() && WarningAsErrorFilter->contains(CheckName)) {
+Level = DiagnosticsEngine::Error;

ilya-biryukov wrote:
> Why can't we call `CTContext->treatAsError()` here?
Good point, that makes this much easier. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61841



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

[PATCH] D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy

[Nathan Ridge via Phabricator via cfe-commits]

nridge updated this revision to Diff 199552.
nridge marked 2 inline comments as done.
nridge added a comment.

Address review comment


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61841

Files:
  clang-tools-extra/clangd/ClangdUnit.cpp


Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -304,6 +304,21 @@
 this->CTContext = CTContext;
   }
 
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+const clang::Diagnostic ) override {
+DiagnosticsEngine::Level Level = DiagLevel;
+
+// Check for warning-as-error.
+if (CTContext && DiagLevel == DiagnosticsEngine::Warning) {
+  std::string CheckName = CTContext->getCheckName(Info.getID());
+  if (!CheckName.empty() && CTContext->treatAsError(CheckName)) {
+Level = DiagnosticsEngine::Error;
+  }
+}
+
+StoreDiags::HandleDiagnostic(Level, Info);
+  }
+
 private:
   tidy::ClangTidyContext *CTContext = nullptr;
 };


Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -304,6 +304,21 @@
 this->CTContext = CTContext;
   }
 
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+const clang::Diagnostic ) override {
+DiagnosticsEngine::Level Level = DiagLevel;
+
+// Check for warning-as-error.
+if (CTContext && DiagLevel == DiagnosticsEngine::Warning) {
+  std::string CheckName = CTContext->getCheckName(Info.getID());
+  if (!CheckName.empty() && CTContext->treatAsError(CheckName)) {
+Level = DiagnosticsEngine::Error;
+  }
+}
+
+StoreDiags::HandleDiagnostic(Level, Info);
+  }
+
 private:
   tidy::ClangTidyContext *CTContext = nullptr;
 };
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.



Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:346
+  std::string CheckName = CTContext->getCheckName(Info.getID());
+  if (!CheckName.empty() && WarningAsErrorFilter->contains(CheckName)) {
+Level = DiagnosticsEngine::Error;

Why can't we call `CTContext->treatAsError()` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61841



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

[PATCH] D61841: [clangd] Respect WarningsAsErrors configuration for clang-tidy

[Nathan Ridge via Phabricator via cfe-commits]

nridge created this revision.
nridge added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, kadircet, arphaman, jkorous, MaskRay.
Herald added a project: clang.

This completes the fix for https://bugs.llvm.org/show_bug.cgi?id=41218.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D61841

Files:
  clang-tools-extra/clangd/ClangdUnit.cpp

Index: clang-tools-extra/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/clangd/ClangdUnit.cpp
+++ clang-tools-extra/clangd/ClangdUnit.cpp
@@ -113,12 +113,12 @@
   }
 
   void EndOfMainFile() {
-for (const auto& Entry : MainFileMacros)
+for (const auto  : MainFileMacros)
   Out->push_back(Entry.getKey());
 llvm::sort(*Out);
   }
 
- private:
+private:
   const SourceManager 
   bool InMainFile = true;
   llvm::StringSet<> MainFileMacros;
@@ -275,12 +275,37 @@
   const LangOptions 
 };
 
+// This is a copy of ClangTidyContext::CachedGlobList.
+// FIXME: Factor out to a common place.
+class CachedGlobList {
+public:
+  CachedGlobList(StringRef Globs) : Globs(Globs) {}
+
+  bool contains(StringRef S) {
+switch (auto  = Cache[S]) {
+case Yes:
+  return true;
+case No:
+  return false;
+case None:
+  Result = Globs.contains(S) ? Yes : No;
+  return Result == Yes;
+}
+llvm_unreachable("invalid enum");
+  }
+
+private:
+  tidy::GlobList Globs;
+  enum Tristate { None, Yes, No };
+  llvm::StringMap Cache;
+};
+
 // A wrapper around StoreDiags to handle suppression comments for
 // clang-tidy diagnostics (and possibly other clang-tidy customizations in the
 // future).
 class ClangdDiagnosticConsumer : public StoreDiags {
 public:
-  ClangdDiagnosticConsumer() {
+  ClangdDiagnosticConsumer(llvm::Optional WarningsAsErrors) {
 filterDiagnostics([this](DiagnosticsEngine::Level DiagLevel,
  const clang::Diagnostic ,
  bool IsInsideMainFile) {
@@ -299,14 +324,36 @@
   }
   return true;
 });
+
+if (WarningsAsErrors) {
+  WarningAsErrorFilter =
+  llvm::make_unique(*WarningsAsErrors);
+}
   }
 
   void setClangTidyContext(tidy::ClangTidyContext *CTContext) {
 this->CTContext = CTContext;
   }
 
+  void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
+const clang::Diagnostic ) override {
+DiagnosticsEngine::Level Level = DiagLevel;
+
+// Check for warning-as-error.
+if (CTContext && WarningAsErrorFilter &&
+DiagLevel == DiagnosticsEngine::Warning) {
+  std::string CheckName = CTContext->getCheckName(Info.getID());
+  if (!CheckName.empty() && WarningAsErrorFilter->contains(CheckName)) {
+Level = DiagnosticsEngine::Error;
+  }
+}
+
+StoreDiags::HandleDiagnostic(Level, Info);
+  }
+
 private:
   tidy::ClangTidyContext *CTContext = nullptr;
+  std::unique_ptr WarningAsErrorFilter;
 };
 
 } // namespace
@@ -328,7 +375,7 @@
   const PrecompiledPreamble *PreamblePCH =
   Preamble ? >Preamble : nullptr;
 
-  ClangdDiagnosticConsumer ASTDiags;
+  ClangdDiagnosticConsumer ASTDiags{Opts.ClangTidyOpts.WarningsAsErrors};
   std::string Content = Buffer->getBuffer();
 
   auto Clang = prepareCompilerInstance(std::move(CI), PreamblePCH,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits