[PATCH] D144748: [clang-tidy] Add bugprone-empty-catch check

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp:100
+void EmptyCatchCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedCatchStmt = Result.Nodes.getNodeAs<CXXCatchStmt>("catch");
+
----------------
Assert that it's not null, or exit early.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EmptyCatchCheck.cpp:104
+      MatchedCatchStmt->getCatchLoc(),
+      "empty catch statements hide issues, to handle exceptions appropriately, "
+      "consider re-throwing, handling, or avoiding catch altogether");
----------------
Nit: I believe a semicolon or dot is more appropriate here


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone/empty-catch.rst:147-153
+.. option:: IgnoreCatchWithKeywords
+
+    This option can be used to ignore specific catch statements containing
+    certain keywords. If a ``catch`` statement body contains (case-insensitive)
+    any of the keywords listed in this semicolon-separated option, then the
+    catch will be ignored, and no warning will be raised.
+    Default value: `@TODO;@FIXME`.
----------------
I'm not sure this option is worth the added complexity - if people want to allow an empty catch with a comment, they might as well just add a `NOLINT` to suppress the warning? It will be even more clear that it's actually a problem: it's not a regular `TODO` that doesn't necessarily imply a problem, but rather something that a static analyzer considers a problem. 




================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp:11
+{
+    try
+    {
----------------
Fix indentation to 2 chars.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone/empty-catch.cpp:16
+    catch(const Exception&)
+// CHECK-MESSAGES: :[[@LINE-1]]:5: warning: empty catch statements hide issues, to handle exceptions appropriately, consider re-throwing, handling, or avoiding catch altogether [bugprone-empty-catch]
+    {
----------------
Align comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144748