NoQ marked an inline comment as done.
NoQ added a comment.
Ouch, seems inline answers don't automatically post themselves when you update
the diff :o
================
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:62
@@ +61,3 @@
+ << "' to a plain boolean value: probably a forgotten "
+ << (IsObjC ? "'[boolValue]'" : "'->isTrue()'");
+ BR.EmitBasicReport(
----------------
dcoughlin wrote:
> - 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.
>
Yeah, noticed the lifetime qualifiers. Changed messages significantly.
================
Comment at: lib/StaticAnalyzer/Checkers/BoolConversionChecker.cpp:117
@@ +116,3 @@
+ auto ConversionThroughComparisonM =
+ binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ hasEitherOperand(NSNumberExprM),
----------------
zaks.anna wrote:
> Can we use any binary operator here without any restrictions?
Comparisons are handled here. Assignments are handled in a different matcher.
Arithmetic operators are either a valid pointer arithmetic (such as `+` or `-`)
or cause compile errors (such as `*` or `/`). Bitwise ops (`&`, `>>`, etc.) -
perhaps somebody is computing shadow memory. Logical ops - also too commonly
intentional (typically: `if (p && [p boolValue])`). Comma operator - ouch, we
should probably warn about the very fact that somebody uses comma operators.
================
Comment at: test/Analysis/bool-conversion.cpp:18
@@ +17,3 @@
+#ifdef PEDANTIC
+ if (p) {} // expected-warning{{}}
+ if (!p) {} // expected-warning{{}}
----------------
zaks.anna wrote:
> 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..
>
> Are these pedantic because we assume these are comparing the pointer and not
> the value? Have you checked that this is a common idiom?
I haven't checked if this is more popular or less popular than `if (p == nil)`,
but it's clearly too popular and too often correct for a warning enabled by
default.
================
Comment at: test/Analysis/bool-conversion.cpp:21
@@ +20,3 @@
+ p ? 1 : 2; // expected-warning{{}}
+ (bool)p; // expected-warning{{}}
+#endif
----------------
zaks.anna wrote:
> Why is this OK?
Will test more closely; i think this gives a lot of intentionals, but a cast to
a non-boolean integer is clearly an error that must be reported.
================
Comment at: test/Analysis/bool-conversion.m:10
@@ +9,3 @@
+ if (!p) {} // expected-warning{{}}
+ if (p == YES) {} // expected-warning{{}}
+ if (p == NO) {} // expected-warning{{}}
----------------
zaks.anna wrote:
> 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.
> There is a Sema warning
Yep, but it is disabled with `-w`. I think it's a good idea to add `-w` to the
run line in all analyzer tests - it often makes tests easier to read, and in
particular we're sure that all warnings in the test are coming from the enabled
checkers, not from other sources.
================
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{{}}
----------------
dcoughlin wrote:
> 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.
Whoops, accidental.
https://reviews.llvm.org/D22968
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
