aaronpuchert added inline comments.
================
Comment at: lib/Analysis/ThreadSafety.cpp:1445
+ if (!TCond && FCond) {
+ Negate = !Negate;
+ return getTrylockCallExpr(COP->getCond(), C, Negate);
----------------
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.
================
Comment at: test/SemaCXX/warn-thread-safety-analysis.cpp:1879-1880
+ void foo13() {
+ if (mu.TryLock() ? 1 : 0)
+ mu.Unlock();
+ }
----------------
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.
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
