aaron.ballman added inline comments.
================
Comment at: lib/Analysis/ThreadSafety.cpp:1445
+ if (!TCond && FCond) {
+ Negate = !Negate;
+ return getTrylockCallExpr(COP->getCond(), C, Negate);
----------------
aaronpuchert wrote:
> aaron.ballman wrote:
> > Rather than do an assignment here, why not just pass `!Negate` directly
> > below, since you're returning?
> The third parameter of `getTrylockCallExpr` is a (non-const) reference, so I
> can only pass an lvalue. It's basically a
> [call-by-reference](https://en.wikipedia.org/wiki/Evaluation_strategy#Call_by_reference)
> pattern.
Ah, I missed that when looking at the code in Phab, thanks for the explanation!
================
Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:1879-1880
+ void foo13() {
+ if (mu.TryLock() ? 1 : 0)
+ mu.Unlock();
+ }
----------------
aaronpuchert wrote:
> aaron.ballman wrote:
> > Can you add a test that shows we get it right even if the user does
> > something less than practical, like:
> > ```
> > if (mu.TryLock() ? mu.TryLock() : false); // Warn about double lock
> > if (mu.TryLock() ? mu.Unlock(), 1 : 0)
> > mu.Unlock(); // Warn about unlocking unheld lock
> > if (mu.TryLock() ? 1 : mu.Unlock(), 0)
> > mu.Unlock(); // Warn about unlocking an unheld lock
> > if (mu.TryLock() ? (true ? mu.TryLock() : false) : false); // Warn about
> > double lock
> > ```
> I'm afraid these don't work as expected if we don't branch on `?:` as before.
> Basically we treat this as if both conditions where evaluated unconditionally.
>
> On calling a try-lock function, no lock is immediately acquired. Only when we
> branch on the result of that try-lock call is the mutex added to the
> capability set on the appropriate branch. Now if we branch on `?:` (the
> situation before this change), we are going to complain about conditionally
> held locks on the join point in @hokein's use case, and if we don't branch
> (the behavior after this change), we are not treating your examples
> correctly. I'm not sure we can treat both as intended.
>
> I'm slightly leaning towards not branching: the C++ standard says that only
> the [used expression is actually
> evaluated](https://en.cppreference.com/w/cpp/language/operator_other#Conditional_operator),
> but I think it's not considered good practice to have side effects in a
> ternary operator. (I can't actually find it anywhere besides a discussion on
> [Hacker News](https://news.ycombinator.com/item?id=14810994). Personally I
> would prefer to use an if-statement if one of the expressions has
> side-effects.)
>
> Tell me what you think.
I agree that we don't want to encourage complex code in ternary operators, but
that complexity does come up often as a result of macro expansion. It feels
like there isn't a good answer here because either approach will have
unexpected results.
I'll let @delesley be the tie-breaker on whether we follow ternary branches or
not, but if we decide to accept this patch and not follow branches, I think we
should capture those test cases as expected false negatives with a comment
about why they happen and why it's expected behavior.
Related, does this patch then impact the behavior of code like:
```
void f(Mutex &M) {
M.Lock();
}
void g() {
Mutex mu;
(void)(true ? f(mu) : (void)0);
mu.Unlock(); // Ok
}
```
Repository:
rC Clang
https://reviews.llvm.org/D52888
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
