[PATCH] D37809: [analyzer] PthreadLock: Refactor, use PostCall API. NFC.

[Aleksei Sidorin via Phabricator via cfe-commits]

a.sidorin added a comment.

I like this refactoring. I wrote some things that are not clear for me inline.



================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:107
+  void TryXNULock(const CallEvent &Call, CheckerContext &C) const;
+  void AcquireLockAux(const CallEvent &Call, CheckerContext &C, unsigned ArgNo,
+                      SVal lock, bool isTryLock,
----------------
In the code below `lock` is always `Call.getSVal(ArgNo)`. Should we remove it and put SVal acquisition into callees?


================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:108
+  void AcquireLockAux(const CallEvent &Call, CheckerContext &C, unsigned ArgNo,
+                      SVal lock, bool isTryLock,
+                      enum LockingSemantics semantics) const;
----------------
According to naming conventions , it should be Lock and IsTryLock. But I'm not sure that this corresponds to the file's code style so I do not insist.


================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:188
+    if (Call.isCalled(I.Name))
+      (this->*I.Callback)(Call, C);
+  }
----------------
break?


================
Comment at: lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp:262
+                                            CheckerContext &C) const {
+  AcquireLockAux(Call, C, 0, Call.getArgSVal(0), false, PthreadSemantics);
+}
----------------
Could you add comments describing what arguments should do? I.e. `/* ArgNo= */ 0, /* Lock= */...? It's hard to remember what do these parameters correspond to.


https://reviews.llvm.org/D37809