[PATCH] D153132: [clang analysis][NFCI] Preparatory work for D153131.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

> Is this actually required for the subsequent change? I don't see the connection.

In the followup change, we have to check the returns after the enter and exit CFG block are computed. We can't analyze the returns as they are seen because , because what matters for the returns is the locks that are live at the end of the function, not those that are live at the point where the `return` happens.

The `BuildLockset` class only lives for the duration of the analysis of a single block, while the `ThreadSafetyAnalyzer` lives for the whole function. So return checking is done in the `ThreadSafetyAnalyzer`, so we need the check/warn functions to be available here.

>From a design perspective I think it might actually make more sens for them to be in the analyzer as `warnIfMutexNotHeld` and friends actually inspects quite a lot of the `Analyzer` state.



================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1541
 class BuildLockset : public ConstStmtVisitor<BuildLockset> {
+  using VisitorBase = ConstStmtVisitor<BuildLockset>;
   friend class ThreadSafetyAnalyzer;
----------------
aaronpuchert wrote:
> Why the alias? I find this just obfuscates.
I'm using it one more time in the followup patch, but no strong opinion, removed.


================
Comment at: clang/lib/Analysis/ThreadSafety.cpp:1588
 /// of at least the passed in AccessKind.
-void BuildLockset::warnIfMutexNotHeld(const NamedDecl *D, const Expr *Exp,
-                                      AccessKind AK, Expr *MutexExp,
-                                      ProtectedOperationKind POK,
-                                      til::LiteralPtr *Self,
-                                      SourceLocation Loc) {
+void ThreadSafetyAnalyzer::warnIfMutexNotHeld(
+    const FactSet &FSet, const NamedDecl *D, const Expr *Exp, AccessKind AK,
----------------
aaronpuchert wrote:
> Hmm, functions of the same class naturally want to be together, but if you move them, it "destroys" the Git history.
Yes, I decided to make review/history tracking esaier by not moving them, but I can move them if you want.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D153132/new/

https://reviews.llvm.org/D153132