[PATCH] D27773: [analyzer] Add checker modeling gtest APIs.

[Anna Zaks via Phabricator via cfe-commits]

zaks.anna added a comment.

Looks great overall. I have minor comments below. Thanks for the awesome comments!!!



================
Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:152
+
+  ProgramStateRef State = C.getState();
+
----------------
This could be moved up as you are using the state on the previous line.


================
Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:159
+
+  State = assumeValuesEqual(ThisSuccess, BooleanArgVal, State, C);
+  C.addTransition(State);
----------------
What happens when they are known not to be equal? I am guessing the transition is just not added, correct? Do you test for that (I did not check.)?


================
Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:172
+    const CXXConstructorCall *Call, CheckerContext &C) const {
+  assert(Call->getNumArgs() > 0);
+
----------------
"getNumArgs() == 1" ?


================
Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:209
+
+  if (CtorDecl->getNumParams() == 0)
+    return;
----------------
It seems that the num of parameter checking logic here is less restrictive than it could be and that makes this a bit hard to read without looking into the 'model' methods. Ex: I think there are 2 cases: 
- Constructor taking a bool can have either 1 or 2 arguments.
- Copy constructor taking exactly one argument.




https://reviews.llvm.org/D27773