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

[Valeriy Savchenko via Phabricator via cfe-commits]

vsavchenko marked 2 inline comments as done.
vsavchenko added inline comments.


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:76
+
+    // Defined elements should maintain the following properties:
+    //   1. for any Kind K: NoReturn | K == K
----------------
NoQ wrote:
> Maybe `static_assert` at least some of those?
That would've been awesome, and I spent some time on figuring out what would be a nice way of doing that.  However, what hurts the most to put at least few of those, is the fact that `operator |` is not declared as `constexpr`.


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:421
+  llvm::Optional<Clarification> VisitBinaryOperator(const BinaryOperator *) {
+    // We don't want to report on short-curcuit logical operations.
+    return llvm::None;
----------------
NoQ wrote:
> Why not? What if the programmer was a big fan of shell scripts?
The only situation where this can be useful if the user has something like this:

```
if (cond1 && cond2 && [obj methodWithCompletionHandler:completionHandler]) { ... }
```

which is way less common (I didn't see that) than a very simple:
```
if (cond1 && cond2 && ...) {
  completionHandler(...);
}
```
And it is pretty frustrating to see `N` warnings on the same if statement in the latter case.  That's why I decided to ignore them for reporting purposes.


================
Comment at: clang/lib/Analysis/CalledOnceCheck.cpp:426
+  llvm::Optional<Clarification> VisitStmt(const Stmt *Terminator) {
+    // If we got here, we didn't have a visit function for more dirived
+    // classes of statement that this terminator actually belongs to.
----------------
NoQ wrote:
> Also there's no test for this, right? Otherwise you would have come up with a better message and this case would have been unnecessary.
Exactly!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92039/new/

https://reviews.llvm.org/D92039