delesley added inline comments.
================
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:4570
mu_.AssertHeld();
- mu_.Unlock();
- } // should this be a warning?
+ mu_.Unlock(); // should this be a warning?
+ }
----------------
This function should have a warning because is not marked with UNLOCK_FUNCTION.
The lock was held on entry, and not held on exit. So the original location of
the comment, on the closing curly brace, was correct.
================
Comment at: clang/test/SemaCXX/warn-thread-safety-analysis.cpp:4629
+ mu_.AssertHeld(); // expected-note {{the other acquisition of mutex
'mu_' is here}}
+ // FIXME: should instead warn because it's unclear whether we need to
release or not.
+ int b = a;
----------------
I'm not wild about having FIXMEs in test code. I would prefer to have the
patch actually issue a warning here (but see below).
HOWEVER, does the current code issue a warning in this situation? If not,
leave it as-is and keep the FIXME. You can't make the analysis more strict
without checking with the C++ compiler team at Google (which I am no longer
on), because it may break the build on our end.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102026/new/
https://reviews.llvm.org/D102026
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
