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

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:41
 
+bool isInNoNamespace(const FunctionDecl *FD) {
+  const DeclContext *DC = FD->getDeclContext();
----------------
`isInGlobalNamespace` might be a better name?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:126-129
+  if (LangOpts.CPlusPlus17)
     return false;
 
   return true;
----------------
`return LangOpts.CPlusPlus17 == 0;`


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:142-149
   Finder->addMatcher(
       callExpr(callee(SignalFunction), hasArgument(1, HandlerExpr))
           .bind("register_call"),
       this);
+  Finder->addMatcher(
+      callExpr(callee(SignalFunction), hasArgument(1, HandlerLambda))
+          .bind("register_call"),
----------------
Is there any utility/performance to be gained by combining these into a single matcher?
e.g.
```
callExpr(callee(SignalFunction), anyOf(hasArgument(1, HandlerExpr), hasArgument(1, HandlerLambda)))
```
(not sure if that is syntactically valid, but you get my point)


================
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 {
----------------
Style guide says no braces on single statement blocks


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:247
 
+  if (!getLangOpts().CPlusPlus17 && getLangOpts().CPlusPlus)
+    return checkFunctionCPP14(FD, CallOrRef, ChainReporter);
----------------
How can the first expression here ever be false when
we rejected C++17 in the `isLanguageVersionSupported`
override?


================
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")) {
----------------
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?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:323-325
+    if (SkipPathEnd) {
+      SkipPathEnd = false;
+    } else {
----------------
drop braces on 'then' block per style guide


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:334-338
+  if (!SkipPathEnd) {
+    diag(HandlerRef->getBeginLoc(),
+         "function %0 registered here as signal handler", DiagnosticIDs::Note)
+        << cast<FunctionDecl>(Caller->getDecl());
+  }
----------------
drop braces per style guide


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118996