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

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

balazske marked 29 inline comments as done.
balazske added a comment.

Code is updated, documentation (rst) not yet. I want to use `\` format of tags in the code comments. Many review comments are now out of place (because the list of functions was moved around in the file.)



================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:94
 
+SourceRange getSourceRangeOfStmt(const Stmt *S, ASTContext &Ctx) {
+  ParentMapContext &PM = Ctx.getParentMapContext();
----------------
whisperity wrote:
> Okay, this is interesting... Why is `Ctx` not `const`? Unless **`get`**`Something()` is changing things (which is non-intuitive then...), as far as I can tell, you're only reading from `Ctx`.
It does only reading, but `getParentMapContext` does not work with `const`.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:96
+  ParentMapContext &PM = Ctx.getParentMapContext();
+  DynTypedNode P = DynTypedNode::create(*S);
+  while (P.getSourceRange().getBegin().isInvalid()) {
----------------
whisperity wrote:
> Are these objects cleaned up properly in their destructor (same question for `DynTypeNodeList`)?
I think it should work if there is not a function for deallocating.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:99-101
+    if (PL.size() != 1)
+      return {};
+    P = PL[0];
----------------
whisperity wrote:
> `size != 1` could still mean `size == 0` in which case taking an element seems dangerous. Is triggering such UB possible here?
size is exactly 1 after the `if`


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:180
+    // No need to display a call chain and no need for more checks.
+    (void)checkFunction(HandlerDecl, HandlerExpr, [](bool) {});
     return;
----------------
whisperity wrote:
> What is happening here? Why is the last parameter a callback(?) taking a `bool`? Why is the lambda empty? I see it is a `std::function`, can't we instead pass an empty function object there, and guard against calling it in `checkFunction`, making things a bit more explicit?
Documentation of the function `checkFunction` tells what is the last parameter for: It is used to display the call chain notes (works as callback). A function object is used because then more information can be passed into it (instead of additional parameters to `checkFunction`) and it is possible to pass an empty function if needed. Here this function is not used, it is empty.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:280-281
+          << R;
+      diag(R.getBegin(), "statement class is: '%0'", DiagnosticIDs::Note)
+          << Name;
+      ChainReporter(/*SkipPathEnd=*/false);
----------------
whisperity wrote:
> I think this is a fine moment to use the `Remark` diagnostic category! ✨  It is very likely that the average client will not look into Clang internals here... (It is dubious whether or not emitting this diagnostic is meaningful in the first place, but at least it helps with //some// debugging/understanding for those who wish to dig deep.)
The statement class is often relatively easy to understand without looking into source code. Still it is a hint only, but there is no easy way to get a description for any statement class (without hard-coding into the checker).


================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.h:39-40
   /// Should not check function calls in the code (this part is done by the call
-  /// graph scan).
+  /// graph scan). If a problem is found in the "function properties" (other
+  /// than the code body) no more problems are to be reported. Otherwise every
+  /// found problem in the code body should be reported (not only the first
----------------
whisperity wrote:
> What is a "function property"? (Or is that an implementation detail of the check?)
I meant a property of the function in everyday sense, for example is it "extern C" or not, `const` or not, and similar (anything that is not in the body part).


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/bugprone-signal-handler.c:21
   // CHECK-NOTES: :[[@LINE-1]]:3: warning: system call 'printf' may not be asynchronous-safe; calling it from a signal handler may be dangerous [bugprone-signal-handler]
-  // CHECK-NOTES: :[[@LINE-2]]:3: note: function 'printf' called here from 'handler_printf'
   // CHECK-NOTES: :[[@LINE+4]]:18: note: function 'handler_printf' registered here as signal handler
----------------
whisperity wrote:
> I really like that this trivial bit stops being repeated! Is this what the `SkipPathEnd` does in practice?
Yes `SkipPathEnd` is added to remove these redundant notes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118996