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