dcoughlin added inline comments. ================ Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:62 @@ +61,3 @@ + << "' to a plain boolean value: probably a forgotten " + << (IsObjC ? "'[boolValue]'" : "'->isTrue()'"); + BR.EmitBasicReport( ---------------- - The '[boolValue]' thing here is weird. Maybe use '-boolValue' instead? - How about "Comparison between 'NSNumber *' and 'BOOL'; use -boolValue instead." - You probably want to remove lifetime qualifiers from the type with `.getUnqualifiedType()`. before printing (i.e., don't mention '__strong') in the diagnostic.
================ Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:80 @@ +79,3 @@ + + auto OSBooleanExprM = + expr(ignoringParenImpCasts( ---------------- The AST matchers seem pretty convenient! ================ Comment at: test/Analysis/bool-conversion.cpp:18 @@ +17,3 @@ +#ifdef PEDANTIC + if (p) {} // expected-warning{{}} + if (!p) {} // expected-warning{{}} ---------------- It is generally good to include the diagnostic text in the test for the warning. This way we make sure we get the warning we expected. ================ Comment at: test/Analysis/bool-conversion.m:2 @@ +1,3 @@ +// RUN: %clang_cc1 -fblocks -w -analyze -analyzer-checker=alpha.osx.BoolConversion %s -verify +// RUN: %clang_cc1 -fblocks -w -analyze -analyzer-checker=alpha.osx.BoolConversion -analyzer-config alpha.osx.BoolConversion:Pedantic=true -DPEDANTIC %s -verify + ---------------- You should add a test invocation here where -fobjc-arc is set as well. This adds a bunch of implicit goop that it would be good to test with. ================ Comment at: test/Analysis/bool-conversion.m:6 @@ +5,3 @@ + +void bad(const NSNumber *p) { +#ifdef PEDANTIC ---------------- These 'const's are not idiomatic. It is probably better to remove them. ================ Comment at: test/Analysis/bool-conversion.m:10 @@ +9,3 @@ + if (!p) {} // expected-warning{{}} + if (p == YES) {} // expected-warning{{}} + if (p == NO) {} // expected-warning{{}} ---------------- There is a Sema warning for `p == YES` already, right? ("comparison between pointer and integer ('NSNumber *' and 'int')") ================ Comment at: test/Analysis/bool-conversion.m:11 @@ +10,3 @@ + if (p == YES) {} // expected-warning{{}} + if (p == NO) {} // expected-warning{{}} + (!p) ? 1 : 2; // expected-warning{{}} ---------------- Should `p == NO` be on in non-pedantic mode, as well? It also seems to me that you could warn on comparison of *any* ObjCObjectPointerType type to `NO`. At a minimum it would probably be good to check for comparisons of `id` to NO. https://reviews.llvm.org/D22968 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits