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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
