[PATCH] D92039: [-Wcalled-once-parameter] Introduce 'called_once' attribute

[Valeriy Savchenko via Phabricator via cfe-commits]

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