aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:199-200
+ const FunctionDecl *HandlerDecl, const Expr *HandlerRef) {
+ int CallLevel = Itr.getPathLength() - 2;
+ const CallGraphNode *Caller = Itr.getPath(CallLevel + 1), *Callee = nullptr;
+ while (CallLevel >= 0) {
----------------
balazske wrote:
> aaron.ballman wrote:
> > Do we have to worry about `CallLevel + 1` being negative here? (Is there a
> > need for an assert?)
> Really yes, but here is no assert because it is on line 161 and the function
> is called only there. A `df_iterator` path contains the start and end node,
> and there should be at least one function if we have any report do show, the
> path length should be at least 1 (make assert for that?). (Later there could
> be cases when the function is called with path length 1.)
Thanks for the explanation -- an assert that the path has at least 1 element
wouldn't be awful, but also doesn't seem like something we'd get a lot of value
from here. It's up to you if you want to add an assert or not.
================
Comment at: clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp:207-208
+ DiagnosticIDs::Note)
+ << cast<FunctionDecl>(Callee->getDecl())
+ << cast<FunctionDecl>(Caller->getDecl());
+ --CallLevel;
----------------
balazske wrote:
> aaron.ballman wrote:
> > Do we have to worry about call expressions for which we cannot find the
> > declaration (like a call through a function pointer)? (Should we add some
> > test coverage involving a call stack with a call through a function
> > pointer?)
> A declaration should always be there in the `CallGraphNode`, probably not a
> definition. The call graph does not insert nodes for function pointer calls.
Oh! I didn't know the CallGraph didn't model calls through function pointers (I
had assumed we would model that because there's still a `CallExpr` involved.)
Can you add a test case involving a call stack with a call through a function
pointer (with comments), just to demonstrate the behavior?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D118224/new/
https://reviews.llvm.org/D118224
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
