[PATCH] D118996: [clang-tidy] Support C++14 in bugprone-signal-handler.

[Balázs Kéri via Phabricator via cfe-commits]

balazske marked 2 inline comments as done.
balazske added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:207-215
+                        })) {
+        // Found problems in this function, skip functions called from it.
+        Itr.skipChildren();
+      } else {
+        ++Itr;
+      }
+    } else {
----------------
LegalizeAdulthood wrote:
> Style guide says no braces on single statement blocks
Improved the code but I think if an `if` branch has braces then the `else` should have too.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:247
 
+  if (!getLangOpts().CPlusPlus17 && getLangOpts().CPlusPlus)
+    return checkFunctionCPP14(FD, CallOrRef, ChainReporter);
----------------
LegalizeAdulthood wrote:
> How can the first expression here ever be false when
> we rejected C++17 in the `isLanguageVersionSupported`
> override?
I was thinking for the case when this check supports C++17 too. The change can be added later, this makes current code more readable.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:275
+    const auto *FoundS = Match.getNodeAs<Stmt>("stmt");
+    StringRef Name = FoundS->getStmtClassName();
+    if (Name.startswith("CXX")) {
----------------
LegalizeAdulthood wrote:
> Hrm, I was curious what `getStmtClassName()` does, but there doesn't seem to be any doxygen on it or the underlying data structure it accesses.  Drilling into it with the IDE seems to say that it's a bunch of enums and corresponding names for each statement kind?
It should return the statement class name, comes somehow from TableGen. Here if the class name contains `CXX` it is recognized as a C++ statement. This check is not fully accurate (for example `LambdaExpr` is missing) but there is likely some other `CXX` statement around.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:323-325
+    if (SkipPathEnd) {
+      SkipPathEnd = false;
+    } else {
----------------
LegalizeAdulthood wrote:
> drop braces on 'then' block per style guide
I think that readability does not get better if the braces are removed here, specially if only from one branch. The rules [[ https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements | here ]] are not strict in this case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118996