[PATCH] D32449: Modifying PthreadLockChecker.cpp to reduce false positives.

[Malhar Thakkar via Phabricator via cfe-commits]

malhar1995 marked 2 inline comments as done.
malhar1995 added a comment.

In https://reviews.llvm.org/D32449#760141, @NoQ wrote:

> Thanks! Your code looks very clear now, and it seems correct to me.
>
> One last thing we definitely should do here would be add regression tests for the new functionality. I guess you already have your tests, otherwise you wouldn't know if your code works, so you'd just need to append them to the patch, somewhere at the bottom of `test/Analysis/pthreadlock.c`, and make sure that `make -j4 check-clang-analysis` passes. Ideally, we should cover as many branches as possible.
>
> A few ideas of what to test (you might have thought about most of them already, and i expect them to actually work by just looking at what your code accomplishes):
>
> - What we can/cannot do with the mutex in the failed-to-be-destroyed state, depending on the state of the mutex before destruction was attempted.
> - What we can/cannot do with the mutex in each of the "Schrodinger" states - in particular, do we display the double-destroy warning in such cases?
> - How return-symbol death races against resolving success of the destroy operation: what if the programmer first tries to destroy mutex, then uses the mutex, then checks the return value?
> - Are you sure we cannot `assert(lstate)` on line 137? - a test could be added that would cause such assertion to fail if someone tries to impose it.
>
>   Apart from that, i guess it'd be good to use more informative variable names in a few places (see inline comments), and fix the formatting, i.e. spaces and line breaks (should be easy with `clang-format`). Also you shouldn't add the `.DS_Store` files :) And then we'd accept and commit this patch.


Dear Dr. Artem,

Thank you so much for such a detailed review. I'll work on addressing these comments ASAP and reach out to you in case I have any queries.

Regards,
Malhar Thakkar


Repository:
  rL LLVM

https://reviews.llvm.org/D32449