vsavchenko added inline comments.
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:163
+ }
+ static bool canBeCalled(Kind K) {
+ return (K & DefinitelyCalled) == DefinitelyCalled && K != Reported;
----------------
NoQ wrote:
> "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.
`DefinitelyCalled` is already a mouthful, so I prefer to keep it together with
`MaybeCalled`. Also I agree about `canBeCalled` and I like your alternative!
================
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:
----------------
NoQ wrote:
> 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).
I mean, we can add `case UO_AddrOf:`, but I don't think that we need more
complex logic (in terms of `*` and `&`) here.
================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:321-322
+ case BO_NE: {
+ const DeclRefExpr *LHS = Visit(BO->getLHS());
+ return LHS ? LHS : Visit(BO->getRHS());
+ }
----------------
NoQ wrote:
> 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)
Yep, I thought about it and, honestly, decided to ignore 😊
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
