NoQ added a comment.
In D92039#2463039 <https://reviews.llvm.org/D92039#2463039>, @vsavchenko wrote:
> Add one gigantic comment on status kinds and the reasons behind some of the
> design choices
This was freakin' awesome, thanks a lot. With all the background in place, the
rest of the patch was a really nice read. I expected it to be a lot more hacky
but it turned out very sane to be honest. I only have minor remarks here and
there.
================
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.
----------------
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"?
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:67
+ // One of the erroneous situations is the case when parameter is called only
+ // on some of the paths. We couldv'e considered it `NotCalled`, but we want
+ // to report double call warnings even if these two calls are not guaranteed
----------------
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:83-84
+ //
+ // Our analysis is intra-procedural and, while in the perfect world,
+ // developers only use tracked parameters to call them, in the real world,
+ // the picture might be different. Parameters can be stored in global
----------------
I think there's nothing wrong with / shameful about escaping, so i wouldn't
call the world without escapes a perfect world, it's unnecessarily constrained
:)
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:163
+ }
+ static bool canBeCalled(Kind K) {
+ return (K & DefinitelyCalled) == DefinitelyCalled && K != Reported;
----------------
"Can be called" sounds like "Someone's allowed to call it". I was completely
confused when i saw a call site for that function.
I too am at loss because the concept of "is definitely potentially called" is
hard to explain especially when the concept of "maybe called" is already
defined and means something different.
Maybe `seenAnyCalls()` or something like that?
Maybe also rename `DefinitelyCalled` and `MaybeCalled` to `CalledOnAllPaths`
and `CalledOnSomePathsOnly`? That could eliminate a bit more confusion.
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:304-307
+ // Function pointer/references can be dereferenced before a call.
+ // That doesn't make it, however, any different from a regular call.
+ // For this reason, dereference operation is a "no-op".
+ case UO_Deref:
----------------
Technically the same applies to operator `&`, eg. `(&*foo)()` is your normal
call. Obviously, `(&foo)()` doesn't compile. That said, `if (&foo)` compiles
and is not your normal null check (but `if (&*foo)` and `if (*foo)` are).
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:321-322
+ case BO_NE: {
+ const DeclRefExpr *LHS = Visit(BO->getLHS());
+ return LHS ? LHS : Visit(BO->getRHS());
+ }
----------------
You're potentially dropping some `DeclRefExpr`s. Also the ones that you find
aren't necessarily the function pointer variables you're looking for (as you
don't actually have any checks in `VisitDeclRefExpr`). Combined, i suspect that
there may be some exotic counterexamples like
```lang=c
if (!foo) {
// "foo" is found
if (foo == completion_handler) {
// ...
}
}
```
(which is probably ok anyway)
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:541-542
+private:
+ NotCalledClarifier(const CFGBlock *Parent, const CFGBlock *SuccInQuestion)
+ : Parent(Parent), SuccInQuestion(SuccInQuestion) {}
+
----------------
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.
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:560
+ bool CheckConventionalParameters)
+ : FunctionCFG(FunctionCFG), AC(AC), Handler(Handler),
+ CheckConventionalParameters(CheckConventionalParameters),
----------------
Why not `FunctionCFG(AC->getCFG())` and omit the CFG parameter? That's the same
function, right?
================
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,
----------------
I'm super used to `V` and `E` for Vertices and Edges in the big-O-notation for
graph algorithms, is it just me?
================
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) {
----------------
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.
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1124
+ (ReturnChildren &&
+ ReturnChildren->getParent(SuspiciousStmt));
+ }
----------------
================
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 {
----------------
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?
================
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
----------------
(:
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1310
+ /// a specified status for the given parameter.
+ bool isAnyOfSuccessorsHasStatus(const CFGBlock *Parent,
+ unsigned ParameterIndex,
----------------
"Successor is has status" doesn't sound right, maybe `anySuccessorHasStatus`?
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:1442
+ // is intentionally not called on this path.
+ if (Cast->getType() == AC.getASTContext().VoidTy) {
+ checkEscapee(Cast->getSubExpr());
----------------
I feel really bad bringing this up in this context but i guess it kind of makes
sense to canonicalize the type first?
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
