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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
