vsavchenko added inline comments.
================
Comment at: clang/include/clang/Analysis/Analyses/CalledOnceCheck.h:32
+/// \enum SwitchSkipped -- there is no call if none of the cases appies.
+/// \enum LoopEntered -- no call when the loop is entered.
+/// \enum LoopSkipped -- no call when the loop is not entered.
----------------
NoQ wrote:
> Hold up, how can this happen at all without path sensitivity? If the loop is
> not entered then literally nothing happens, so there can't be a call. If
> there's no call in either case then probably it's just "there's simply no
> call at all"?
You can see examples in tests 😉
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:541-542
+private:
+ NotCalledClarifier(const CFGBlock *Parent, const CFGBlock *SuccInQuestion)
+ : Parent(Parent), SuccInQuestion(SuccInQuestion) {}
+
----------------
NoQ wrote:
> This is one of the more subtle facilities, i really want a comment here a lot
> more than on the `NamesCollector`. Like, which blocks do you feed into this
> class? Do i understand correctly that `Parent` is the block at which we
> decided to emit the warning? And `SuccInQuestion` is its successor on which
> there was no call? Or on which there was a call? I can probably ultimately
> figure this out if i read all the code but i would be much happier if there
> was a comment.
These are correct questions and I will add a comment, but I want to point out
that these questions should be asked not about a private constructor (I always
consider those as "don't go there girlfriend!"). The only "entry point" to
this class,`clarify`, has more reasonable parameter names.
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:560
+ bool CheckConventionalParameters)
+ : FunctionCFG(FunctionCFG), AC(AC), Handler(Handler),
+ CheckConventionalParameters(CheckConventionalParameters),
----------------
NoQ wrote:
> Why not `FunctionCFG(AC->getCFG())` and omit the CFG parameter? That's the
> same function, right?
Right, I thought about it! It is simply how it was done in some other
analysis-based warning
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:780
+ //
+ // The following algorithm has the worst case complexity of O(N + E),
+ // where N is the number of basic blocks in FunctionCFG,
----------------
NoQ wrote:
> I'm super used to `V` and `E` for Vertices and Edges in the big-O-notation
> for graph algorithms, is it just me?
Yep, it is a rudiment of the previous version of this comment. I'll change it!
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:822
+ /// calling the parameter itself, but rather uses it as the argument.
+ template <class CallLikeExpr>
+ void checkIndirectCall(const CallLikeExpr *CallOrMessage) {
----------------
NoQ wrote:
> Did you consider `AnyCall`? That's a universal wrapper for all kinds of AST
> calls for exactly these cases. It's not super compile-time but it adds a lot
> of convenience. (Also uh-oh, its doxygen page seems to be broken). It's ok if
> you find it unsuitable but i kind of still want to popularize it.
It doesn't seem to have iteration over arguments.
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1124
+ (ReturnChildren &&
+ ReturnChildren->getParent(SuspiciousStmt));
+ }
----------------
NoQ wrote:
>
Oh, didn't notice that one!
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1132
+ /// Check if parameter with the given index was ever used.
+ /// NOTE: This function checks only for escapes.
+ bool wasEverUsed(unsigned Index) const {
----------------
NoQ wrote:
> So the contract of this function is "We haven't seen any actual calls, which
> is why we're wondering", otherwise it's not expected to do what it says it
> does?
Right!
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1247
+ State &ToAlter) const {
+ // Even when the analyzer is technically correct, it is a widespread
pattern
+ // not to call completion handlers in some scenarios. These usually have
----------------
NoQ wrote:
> (:
So true 😄
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1310
+ /// a specified status for the given parameter.
+ bool isAnyOfSuccessorsHasStatus(const CFGBlock *Parent,
+ unsigned ParameterIndex,
----------------
NoQ wrote:
> "Successor is has status" doesn't sound right, maybe `anySuccessorHasStatus`?
Dern it! I noticed that before, but forgot to change it, thanks!
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1442
+ // is intentionally not called on this path.
+ if (Cast->getType() == AC.getASTContext().VoidTy) {
+ checkEscapee(Cast->getSubExpr());
----------------
NoQ wrote:
> I feel really bad bringing this up in this context but i guess it kind of
> makes sense to canonicalize the type first?
Will do
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92039/new/
https://reviews.llvm.org/D92039
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
