[PATCH] D113999: [clangd] Fix assertion crashes on unmatched NOLINTBEGIN comments.

[Haojian Wu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4ea066acc928: [clangd] Fix assertion crashes on unmatched 
NOLINTBEGIN comments. (authored by hokein).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113999

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -476,6 +476,13 @@
   double g = [[8]] / i;
   #define BAD2 BAD
   double h = BAD2;  // NOLINT
+  // NOLINTBEGIN
+  double x = BAD2;
+  double y = BAD2;
+  // NOLINTEND
+
+  // verify no crashes on unmatched nolints.
+  // NOLINTBEIGN
 }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -391,9 +391,13 @@
   bool IsInsideMainFile =
   Info.hasSourceManager() &&
   isInsideMainFile(Info.getLocation(), Info.getSourceManager());
+  SmallVector TidySuppressedErrors;
   if (IsInsideMainFile &&
   tidy::shouldSuppressDiagnostic(DiagLevel, Info, *CTContext,
+ TidySuppressedErrors,
  /*AllowIO=*/false)) {
+// FIXME: should we expose the suppression error (invalid use of
+// NOLINT comments)?
 return DiagnosticsEngine::Ignored;
   }
 
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -223,10 +223,6 @@
 /// for example, the use of a "NOLINTBEGIN" comment that is not followed by a
 /// "NOLINTEND" comment - a diagnostic regarding the improper use is returned
 /// via the output argument `SuppressionErrors`.
-bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
-  const Diagnostic , ClangTidyContext 
,
-  bool AllowIO = true);
-
 bool shouldSuppressDiagnostic(
 DiagnosticsEngine::Level DiagLevel, const Diagnostic ,
 ClangTidyContext ,
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -517,16 +517,6 @@
 namespace clang {
 namespace tidy {
 
-bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
-  const Diagnostic , ClangTidyContext 
,
-  bool AllowIO) {
-  SmallVector Unused;
-  bool ShouldSuppress =
-  shouldSuppressDiagnostic(DiagLevel, Info, Context, Unused, AllowIO);
-  assert(Unused.empty());
-  return ShouldSuppress;
-}
-
 bool shouldSuppressDiagnostic(
 DiagnosticsEngine::Level DiagLevel, const Diagnostic ,
 ClangTidyContext ,


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -476,6 +476,13 @@
   double g = [[8]] / i;
   #define BAD2 BAD
   double h = BAD2;  // NOLINT
+  // NOLINTBEGIN
+  double x = BAD2;
+  double y = BAD2;
+  // NOLINTEND
+
+  // verify no crashes on unmatched nolints.
+  // NOLINTBEIGN
 }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -391,9 +391,13 @@
   bool IsInsideMainFile =
   Info.hasSourceManager() &&
   isInsideMainFile(Info.getLocation(), Info.getSourceManager());
+  SmallVector TidySuppressedErrors;
   if (IsInsideMainFile &&
   tidy::shouldSuppressDiagnostic(DiagLevel, Info, *CTContext,
+ TidySuppressedErrors,
  /*AllowIO=*/false)) {
+// FIXME: should we expose the suppression error (invalid use of
+// NOLINT comments)?
 return 

[PATCH] D113999: [clangd] Fix assertion crashes on unmatched NOLINTBEGIN comments.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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

thanks, lgtm!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113999

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

[PATCH] D113999: [clangd] Fix assertion crashes on unmatched NOLINTBEGIN comments.

[Haojian Wu via Phabricator via cfe-commits]

hokein created this revision.
hokein added a reviewer: sammccall.
Herald added subscribers: carlosgalvezp, usaxena95, kadircet, arphaman.
hokein requested review of this revision.
Herald added subscribers: MaskRay, ilya-biryukov.
Herald added a project: clang-tools-extra.

The overload shouldSuppressDiagnostic seems unnecessary, and it is only
used in clangd.

This patch removes it and use the real one (suppression diagnostics are
discarded in clangd at the moment).

Fixes https://github.com/clangd/clangd/issues/929


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D113999

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tools-extra/clangd/ParsedAST.cpp
  clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -476,6 +476,13 @@
   double g = [[8]] / i;
   #define BAD2 BAD
   double h = BAD2;  // NOLINT
+  // NOLINTBEGIN
+  double x = BAD2;
+  double y = BAD2;
+  // NOLINTEND
+
+  // verify no crashes on unmatched nolints.
+  // NOLINTBEIGN
 }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -391,9 +391,13 @@
   bool IsInsideMainFile =
   Info.hasSourceManager() &&
   isInsideMainFile(Info.getLocation(), Info.getSourceManager());
+  SmallVector TidySuppressedErrors;
   if (IsInsideMainFile &&
   tidy::shouldSuppressDiagnostic(DiagLevel, Info, *CTContext,
+ TidySuppressedErrors,
  /*AllowIO=*/false)) {
+// FIXME: should we expose the suppression error (invalid use of
+// NOLINT comments)?
 return DiagnosticsEngine::Ignored;
   }
 
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -228,10 +228,6 @@
 /// for example, the use of a "NOLINTBEGIN" comment that is not followed by a
 /// "NOLINTEND" comment - a diagnostic regarding the improper use is returned
 /// via the output argument `SuppressionErrors`.
-bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
-  const Diagnostic , ClangTidyContext 
,
-  bool AllowIO = true);
-
 bool shouldSuppressDiagnostic(
 DiagnosticsEngine::Level DiagLevel, const Diagnostic ,
 ClangTidyContext ,
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -517,16 +517,6 @@
 namespace clang {
 namespace tidy {
 
-bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
-  const Diagnostic , ClangTidyContext 
,
-  bool AllowIO) {
-  SmallVector Unused;
-  bool ShouldSuppress =
-  shouldSuppressDiagnostic(DiagLevel, Info, Context, Unused, AllowIO);
-  assert(Unused.empty());
-  return ShouldSuppress;
-}
-
 bool shouldSuppressDiagnostic(
 DiagnosticsEngine::Level DiagLevel, const Diagnostic ,
 ClangTidyContext ,


Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -476,6 +476,13 @@
   double g = [[8]] / i;
   #define BAD2 BAD
   double h = BAD2;  // NOLINT
+  // NOLINTBEGIN
+  double x = BAD2;
+  double y = BAD2;
+  // NOLINTEND
+
+  // verify no crashes on unmatched nolints.
+  // NOLINTBEIGN
 }
   )cpp");
   TestTU TU = TestTU::withCode(Main.code());
Index: clang-tools-extra/clangd/ParsedAST.cpp
===
--- clang-tools-extra/clangd/ParsedAST.cpp
+++ clang-tools-extra/clangd/ParsedAST.cpp
@@ -391,9 +391,13 @@
   bool IsInsideMainFile =
   Info.hasSourceManager() &&
   isInsideMainFile(Info.getLocation(), Info.getSourceManager());
+  SmallVector TidySuppressedErrors;
   if (IsInsideMainFile &&
   tidy::shouldSuppressDiagnostic(DiagLevel, Info, *CTContext,
+