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