[PATCH] D52888: Thread safety analysis: Handle conditional expression in getTrylockCallExpr

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D52888#1256625, @aaronpuchert wrote:

> In https://reviews.llvm.org/D52888#1256395, @aaron.ballman wrote:
>
> > In https://reviews.llvm.org/D52888#1255862, @aaronpuchert wrote:
> >
> > > Additional changes (including some non-tail recursion unfortunately) would allow the following to work:
> > >
> > >   void foo16() {
> > >     if (cond ? mu.TryLock() : false)
> > >       mu.Unlock();
> > >   }
> > >   
> > >   void foo17() {
> > >     if (cond ? true : !mu.TryLock())
> > >       return;
> > >     mu.Unlock();
> > >   }
> > >   
> > >
> > > Worth the effort, or is that too exotic?
> >
> >
> > `foo16()` looks like code I could plausibly see in the wild. Consider the case where the mutex is a pointer and you want to test it for null before calling `TryLock()` on it (`M ? M->TryLock() : false`). However, I don't have a good feeling for how much work it would be to support that case.
>
>
> It's relatively short, but not exactly straightforward, because we have to check if both branches are "compatible". However, thinking about this again I think most programmers would write `foo16` as
>
>   void foo16() {
>     if (cond && mu.TryLock())
>       mu.Unlock();
>   }
>
>
> which is functionally equivalent, and which we already support. So I'd propose to add support for this only when someone asks for it, and leave it as it is for now.


I think that's reasonable. LGTM!


Repository:
  rC Clang

https://reviews.llvm.org/D52888