[PATCH] D118370: [clang-tidy] bugprone-signal-handler: Message improvement and code refactoring.

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

balazske added inline comments.
Herald added a project: All.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:159-160
        Itr != ItrE; ++Itr) {
     const auto *CallF = dyn_cast<FunctionDecl>((*Itr)->getDecl());
-    if (CallF && !isFunctionAsyncSafe(CallF)) {
-      assert(Itr.getPathLength() >= 2);
-      reportBug(CallF, findCallExpr(Itr.getPath(Itr.getPathLength() - 2), *Itr),
-                /*DirectHandler=*/false);
-      reportHandlerCommon(Itr, SignalCall, HandlerDecl, HandlerExpr);
+    if (CallF) {
+      unsigned int PathL = Itr.getPathLength();
----------------
whisperity wrote:
> 
Is this code not too long to put in the condition section?


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:165-171
+      if (checkFunction(CallF, CallOrRef))
+        reportHandlerChain(Itr, HandlerExpr);
     }
   }
 }
 
+bool SignalHandlerCheck::checkFunction(const FunctionDecl *FD,
----------------
whisperity wrote:
> What does the return value of `checkFunction` signal to the user, and the `if`?
This has a description in the header file.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler-posix.c:14
   printf("1234");
-  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
 }
----------------
whisperity wrote:
> I'm not exactly sure we should call `printf` (and `memcpy`) a "system call". As far as I can see, in this patch, `isSystemCall()` boils down to //"declaration in file included as system header"//
Call it "standard function"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118370