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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
