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

[Anna Zaks via cfe-commits]

zaks.anna added a comment.

Here are more comments. Could you address/answer these and upload the latest patch that compares NSNumber to other numbers?

Thanks!


================
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:88
@@ +87,3 @@
+
+    auto NSNumberExprM =
+        expr(ignoringParenImpCasts(expr(hasType(hasCanonicalType(
----------------
Can you test if matching for NSNumber works when they come from ivars, properties, and method returns,  works?


================
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:117
@@ +116,3 @@
+    auto ConversionThroughComparisonM =
+        binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+                       hasEitherOperand(NSNumberExprM),
----------------
Can we use any binary operator here without any restrictions?

================
Comment at: test/Analysis/bool-conversion.cpp:18
@@ +17,3 @@
+#ifdef PEDANTIC
+  if (p) {} // expected-warning{{}}
+  if (!p) {} // expected-warning{{}}
----------------
dcoughlin wrote:
> 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.
+1

Are these pedantic because we assume these are comparing the pointer and not the value? Have you checked that this is a common idiom?

Same comment applies to NSNumber. If it is common practice to compare against nil..


================
Comment at: test/Analysis/bool-conversion.cpp:21
@@ +20,3 @@
+  p ? 1 : 2; // expected-warning{{}}
+  (bool)p; // expected-warning{{}}
+#endif
----------------
Why is this OK?

================
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
+
----------------
dcoughlin wrote:
> 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.
+ 1!!!
Especially, since we are matching the AST.

================
Comment at: test/Analysis/bool-conversion.m:10
@@ +9,3 @@
+  if (!p) {} // expected-warning{{}}
+  if (p == YES) {} // expected-warning{{}}
+  if (p == NO) {} // expected-warning{{}}
----------------
dcoughlin wrote:
> There is a Sema warning for `p == YES` already, right? ("comparison between pointer and integer ('NSNumber *' and 'int')")
These tests seem to be even more relevant to OSBoolean.


https://reviews.llvm.org/D22968