[PATCH] D22968: [analyzer] A checker for macOS-specific bool-like objects.

[Devin Coughlin via cfe-commits]

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