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

[Devin Coughlin via Phabricator via cfe-commits]

dcoughlin added a comment.

I already committed this, but I'll address the feedback in a follow-on commit.



================
Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:172
+    const CXXConstructorCall *Call, CheckerContext &C) const {
+  assert(Call->getNumArgs() > 0);
+
----------------
zaks.anna wrote:
> "getNumArgs() == 1" ?
You can have additional arguments to a copy constructor as long as they are defaulted. I was trying to be robust against future source-compatible changes, but this was probably too clever. If the copy constructor changes then maybe it would be better to just not model.


================
Comment at: lib/StaticAnalyzer/Checkers/GTestChecker.cpp:209
+
+  if (CtorDecl->getNumParams() == 0)
+    return;
----------------
zaks.anna wrote:
> 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.
> 
> 
I'll make this more clear.


================
Comment at: test/Driver/analyzer-target-enabled-checkers.cpp:7
 // CHECK-DARWIN: "-analyzer-checker=core"
+// CHECK-DARWIN-SAME: "-analyzer-checker=apiModeling"
 // CHECK-DARWIN-SAME: "-analyzer-checker=unix"
----------------
a.sidorin wrote:
> A very minor nit/question.
> Do we have any convention on checker naming? Most checkers are starting with capital letters, but some not. As "API" is an abbreviation, I think we should at least start it with capital.
We're not terribly consistent, but in general we start packages with lowercase letters and checkers with uppercase names. Checkers are generally UpperCamelCase.

"apiModeling" is a package name, not a checker name. I think to be consistent with most of the other packages it should (?) probably be lowerCamelCase. This matches "coreFoundation" and "insecureAPI" but not  "deadcode". I had "api" lowercase since it is an initialism for a thing (package) that starts with lower case and this is how we treat lower camel-case initialisms in Swift. It also matches "osx", which is similarly an initialism.

Does knowing it is a package name (rather than a checker name) change your opinion of whether "API" should be upper case. I'm happy to change it, since it will not be user facing.



https://reviews.llvm.org/D27773