[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
This revision was automatically updated to reflect the committed changes.
Closed by commit rL285533: [analyzer] NumberObjectConversion: support more
types, misc updates. (authored by dergachev).
Changed prior to commit:
https://reviews.llvm.org/D25731?vs=76043&id=76349#toc
Repository:
rL LLVM
https://reviews.llvm.org/D25731
Files:
cfe/trunk/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
cfe/trunk/test/Analysis/number-object-conversion.c
cfe/trunk/test/Analysis/number-object-conversion.cpp
cfe/trunk/test/Analysis/number-object-conversion.m
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
@@ -63,33 +63,30 @@
} // end of anonymous namespace
void Callback::run(const MatchFinder::MatchResult &Result) {
- bool IsPedanticMatch = (Result.Nodes.getNodeAs("pedantic") != nullptr);
+ bool IsPedanticMatch =
+ (Result.Nodes.getNodeAs("pedantic") != nullptr);
if (IsPedanticMatch && !C->Pedantic)
return;
- const Stmt *Conv = Result.Nodes.getNodeAs("conv");
- assert(Conv);
- const Expr *Osboolean = Result.Nodes.getNodeAs("osboolean");
- const Expr *Nsnumber = Result.Nodes.getNodeAs("nsnumber");
- bool IsObjC = (bool)Nsnumber;
- const Expr *Obj = IsObjC ? Nsnumber : Osboolean;
- assert(Obj);
-
ASTContext &ACtx = ADC->getASTContext();
if (const Expr *CheckIfNull =
Result.Nodes.getNodeAs("check_if_null")) {
-// We consider NULL to be a pointer, even if it is defined as a plain 0.
-// FIXME: Introduce a matcher to implement this logic?
+// Unless the macro indicates that the intended type is clearly not
+// a pointer type, we should avoid warning on comparing pointers
+// to zero literals in non-pedantic mode.
+// FIXME: Introduce an AST matcher to implement the macro-related logic?
+bool MacroIndicatesWeShouldSkipTheCheck = false;
SourceLocation Loc = CheckIfNull->getLocStart();
if (Loc.isMacroID()) {
StringRef MacroName = Lexer::getImmediateMacroName(
Loc, ACtx.getSourceManager(), ACtx.getLangOpts());
- if (MacroName != "YES" && MacroName != "NO")
+ if (MacroName == "NULL" || MacroName == "nil")
return;
-} else {
- // Otherwise, comparison of pointers to 0 might still be intentional.
- // See if this is the case.
+ if (MacroName == "YES" || MacroName == "NO")
+MacroIndicatesWeShouldSkipTheCheck = true;
+}
+if (!MacroIndicatesWeShouldSkipTheCheck) {
llvm::APSInt Result;
if (CheckIfNull->IgnoreParenCasts()->EvaluateAsInt(
Result, ACtx, Expr::SE_AllowSideEffects)) {
@@ -102,33 +99,92 @@
}
}
+ const Stmt *Conv = Result.Nodes.getNodeAs("conv");
+ assert(Conv);
+
+ const Expr *ConvertedCObject = Result.Nodes.getNodeAs("c_object");
+ const Expr *ConvertedCppObject = Result.Nodes.getNodeAs("cpp_object");
+ const Expr *ConvertedObjCObject = Result.Nodes.getNodeAs("objc_object");
+ bool IsCpp = (ConvertedCppObject != nullptr);
+ bool IsObjC = (ConvertedObjCObject != nullptr);
+ const Expr *Obj = IsObjC ? ConvertedObjCObject
+ : IsCpp ? ConvertedCppObject
+ : ConvertedCObject;
+ assert(Obj);
+
+ bool IsComparison =
+ (Result.Nodes.getNodeAs("comparison") != nullptr);
+
+ bool IsOSNumber =
+ (Result.Nodes.getNodeAs("osnumber") != nullptr);
+
+ bool IsInteger =
+ (Result.Nodes.getNodeAs("int_type") != nullptr);
+ bool IsObjCBool =
+ (Result.Nodes.getNodeAs("objc_bool_type") != nullptr);
+ bool IsCppBool =
+ (Result.Nodes.getNodeAs("cpp_bool_type") != nullptr);
+
llvm::SmallString<64> Msg;
llvm::raw_svector_ostream OS(Msg);
- OS << "Converting '"
- << Obj->getType().getCanonicalType().getUnqualifiedType().getAsString()
- << "' to a plain ";
-
- if (Result.Nodes.getNodeAs("int_type") != nullptr)
-OS << "integer value";
- else if (Result.Nodes.getNodeAs("objc_bool_type") != nullptr)
-OS << "BOOL value";
- else if (Result.Nodes.getNodeAs("cpp_bool_type") != nullptr)
-OS << "bool value";
+
+ // Remove ObjC ARC qualifiers.
+ QualType ObjT = Obj->getType().getUnqualifiedType();
+
+ // Remove consts from pointers.
+ if (IsCpp) {
+assert(ObjT.getCanonicalType()->isPointerType());
+ObjT = ACtx.getPointerType(
+ObjT->getPointeeType().getCanonicalType().getUnqualifiedType());
+ }
+
+ if (IsComparison)
+OS << "Comparing ";
else
-OS << "boolean value for branching";
+OS << "Converting ";
- if (IsPedanticMatch) {
-if (IsObjC) {
- OS << "; please compare the pointer to nil instead "
-"to suppress this warning";
-} else {
- OS << "; please compare the pointer to NULL or nullptr instead "
-"to suppress t
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
dcoughlin accepted this revision.
dcoughlin added a comment.
LGTM.
Comment at: test/Analysis/number-object-conversion.m:98
+
+#define NULL_INSIDE_MACRO NULL
+void test_NULL_inside_macro(NSNumber *p) {
This is great! And a good catch.
https://reviews.llvm.org/D25731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
NoQ updated this revision to Diff 76043.
NoQ added a comment.
- Fix warning messages, finally, hopefully.
- Make handling of macros much more careful, because errors of form `x == Y`,
where X is an `NSNumber` pointer, and `Y` is a custom macro that expands to
`0`, were found.
https://reviews.llvm.org/D25731
Files:
lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
test/Analysis/number-object-conversion.c
test/Analysis/number-object-conversion.cpp
test/Analysis/number-object-conversion.m
Index: test/Analysis/number-object-conversion.m
===
--- test/Analysis/number-object-conversion.m
+++ test/Analysis/number-object-conversion.m
@@ -10,30 +10,35 @@
void bad(NSNumber *p) {
#ifdef PEDANTIC
- if (p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- if (!p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- (!p) ? 1 : 2; // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- (BOOL)p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; please compare the pointer to nil instead to suppress this warning}}
- if (p == 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; please compare the pointer to nil instead to suppress this warning}}
- if (p > 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+ if (p) {} // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+ if (!p) {} // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+ (!p) ? 1 : 2; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+ if (p == 0) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a scalar integer value; instead, either compare the pointer to nil or compare the result of calling a method on 'NSNumber *' to get the scalar value}}
+#else
+ if (p) {} // no-warning
+ if (!p) {} // no-warning
+ (!p) ? 1 : 2; // no-warning
+ if (p == 0) {} // no-warning
#endif
- if (p == YES) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- if (p == NO) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- BOOL x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- x = (p == YES); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- if (p == 1) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- int y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- takes_boolean(p); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- takes_integer(p); // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+ (BOOL)p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+ if (p > 0) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a scalar integer value; did you mean to compare the result of calling a method on 'NSNumber *' to get the scalar value?}}
+ if (p == YES) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+ if (p == NO) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+ BOOL x = p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+ x = p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+ x = (p == YES); // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
zaks.anna accepted this revision.
zaks.anna added a comment.
This revision is now accepted and ready to land.
Minor nit below.
Thanks for iterating so much on this!
Anna.
Comment at: test/Analysis/number-object-conversion.cpp:46
+#ifdef PEDANTIC
+ if (p) {} // expected-warning{{Converting a pointer value of type 'class
OSNumber *' to a primitive boolean value; instead, either compare the pointer
to nullptr or call a method on 'class OSNumber *' to get the scalar value}}
+ if (!p) {} // expected-warning{{Converting a pointer value of type 'class
OSNumber *' to a primitive boolean value; instead, either compare the pointer
to nullptr or call a method on 'class OSNumber *' to get the scalar value}}
Minor nit: Here, we want to use scalar instead of primitive so that both parts
of the sentence are consistent. Devin did not like "scalar", but that's what
it is called in the API docs, so I think should use that word in this specific
case. (We can keep 'primitive' for the error messages that do not have "to get
the ... value")
Sorry, this is a bit confusing.
https://reviews.llvm.org/D25731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
NoQ updated this revision to Diff 75612.
NoQ added a comment.
Do not suggest any API when we're not sure (was already advised by Anna but i
missed it somehow).
https://reviews.llvm.org/D25731
Files:
lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
test/Analysis/number-object-conversion.c
test/Analysis/number-object-conversion.cpp
test/Analysis/number-object-conversion.m
Index: test/Analysis/number-object-conversion.m
===
--- test/Analysis/number-object-conversion.m
+++ test/Analysis/number-object-conversion.m
@@ -10,30 +10,35 @@
void bad(NSNumber *p) {
#ifdef PEDANTIC
- if (p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- if (!p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- (!p) ? 1 : 2; // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- (BOOL)p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; please compare the pointer to nil instead to suppress this warning}}
- if (p == 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; please compare the pointer to nil instead to suppress this warning}}
- if (p > 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+ if (p) {} // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+ if (!p) {} // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+ (!p) ? 1 : 2; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+ if (p == 0) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive integer value; instead, either compare the pointer to nil or compare the result of calling a method on 'NSNumber *' to get the scalar value}}
+#else
+ if (p) {} // no-warning
+ if (!p) {} // no-warning
+ (!p) ? 1 : 2; // no-warning
+ if (p == 0) {} // no-warning
#endif
- if (p == YES) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- if (p == NO) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- BOOL x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- x = (p == YES); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- if (p == 1) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- int y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- takes_boolean(p); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- takes_integer(p); // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+ (BOOL)p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+ if (p > 0) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive integer value; did you mean to compare the result of calling a method on 'NSNumber *' to get the scalar value?}}
+ if (p == YES) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+ if (p == NO) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+ BOOL x = p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+ x = p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+ x = (p == YES); // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+ if (p == 1) {} // expected-warning{{Comparing a pointer value of type 'NS
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
NoQ updated this revision to Diff 75586.
NoQ marked an inline comment as done.
NoQ added a comment.
- Update warning messages. I think it's better to pattern-match for integer
sizes after all when we're suggesting API, this especially looks ugly for
OSNumber (which is rare).
- Add tests for converting `CFNumberRef` into `intptr_t`.
- An unrelated change: Move casts to bools to non-pedantic mode (i mistakenly
thought this check is loud).
https://reviews.llvm.org/D25731
Files:
lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
test/Analysis/number-object-conversion.c
test/Analysis/number-object-conversion.cpp
test/Analysis/number-object-conversion.m
Index: test/Analysis/number-object-conversion.m
===
--- test/Analysis/number-object-conversion.m
+++ test/Analysis/number-object-conversion.m
@@ -10,30 +10,35 @@
void bad(NSNumber *p) {
#ifdef PEDANTIC
- if (p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- if (!p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- (!p) ? 1 : 2; // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- (BOOL)p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; please compare the pointer to nil instead to suppress this warning}}
- if (p == 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; please compare the pointer to nil instead to suppress this warning}}
- if (p > 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+ if (p) {} // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+ if (!p) {} // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+ (!p) ? 1 : 2; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive boolean value; instead, either compare the pointer to nil or call -boolValue}}
+ if (p == 0) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive integer value; instead, either compare the pointer to nil or compare the result of calling object API such as -intValue}}
+#else
+ if (p) {} // no-warning
+ if (!p) {} // no-warning
+ (!p) ? 1 : 2; // no-warning
+ if (p == 0) {} // no-warning
#endif
- if (p == YES) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- if (p == NO) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- BOOL x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- x = (p == YES); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- if (p == 1) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- int y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- takes_boolean(p); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- takes_integer(p); // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+ (BOOL)p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+ if (p > 0) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive integer value; did you mean to compare the result of calling object API such as -intValue?}}
+ if (p == YES) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+ if (p == NO) {} // expected-warning{{Comparing a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to compare the result of calling -boolValue?}}
+ BOOL x = p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+ x = p; // expected-warning{{Converting a pointer value of type 'NSNumber *' to a primitive BOOL value; did you mean to call -boolValue?}}
+ x =
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
zaks.anna added inline comments.
Comment at: test/Analysis/number-object-conversion.c:14
+ if (p) {} // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
+ if (!p) {} // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
+ p ? 1 : 2; // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
NoQ wrote:
> zaks.anna wrote:
> > How about:
> > "Converting 'CFNumberRef' pointer to a plain boolean value; instead,
> > compare the pointer to NULL or compare to the encapsulated scalar value"
> >
> > - I've added "pointer".
> > - I would remove "for branching". Does it add anything?
> > - Also, we should remove "please" as it makes the warning text longer.
> >
> > I would remove "for branching". Does it add anything?
>
> Because there's otherwise no obvious boolean value around, i wanted to point
> out what exactly is going on.
>
> > or compare to the encapsulated scalar value
>
> They don't necessarily compare the value. Maybe "take"?
>
> > I've added "pointer".
>
> Not sure it's worth keeping for other objects ("`'NSNumber *' pointer`"
> sounds like a pointer to a pointer).
>> or compare to the encapsulated scalar value
> They don't necessarily compare the value. Maybe "take"?
OSBoolean or CFNumber:
[Comparing|Converting] a pointer value of type '[CFNumberRef|NSNumber *]' to a
scalar [boolean|integer] value; instead, either compare the pointer to
[NULL|nullptr|nil] or [call [APIName] | compare the result of calling [API
Name]].
NSNumber or OSNumber:
Converting a pointer value of type '[NSNumber *]' to a scalar [boolean|integer]
value; instead, either compare the pointer to [NULL|nullptr|nil] or call a
method on '[NSNumber *]' to get the scalar value.
Comparing a pointer value of type '[NSNumber *]' to a scalar [boolean|integer]
value; instead, either compare the pointer to [NULL|nullptr|nil] or call a
method on '[NSNumber *]' to get the scalar value.
Comment at: test/Analysis/number-object-conversion.c:23
+#endif
+ if (p > 0) {} // expected-warning{{Converting 'CFNumberRef' pointer to a
plain integer value; pointer value is being used instead}}
+ int x = p; // expected-warning{{Converting 'CFNumberRef' pointer to a plain
integer value; pointer value is being used instead}}
Comparing a pointer value of type '[CFNumberRef|NSNumber *]' to a primitive
[boolean|integer] value; did you mean to compare the result of calling [API
Name].
Comment at: test/Analysis/number-object-conversion.cpp:32
+ p ? 1 : 2; // expected-warning{{Converting 'const class OSBoolean *' pointer
to a branch condition; instead, compare the pointer to nullptr or take the
encapsulated scalar value}}
+ (bool)p; // expected-warning{{Converting 'const class OSBoolean *' pointer
to a plain bool value; instead, compare the pointer to nullptr or take the
encapsulated scalar value}}
+#else
Converting a pointer value of type '[CFNumberRef|NSNumber *]' to a primitive
[boolean|integer] value; did you mean to call [APIName].
Comment at: test/Analysis/number-object-conversion.cpp:41
+ x = p; // expected-warning{{Converting 'const class OSBoolean *' pointer to
a plain bool value; pointer value is being used instead}}
+ takes_bool(p); // expected-warning{{Converting 'const class OSBoolean *'
pointer to a plain bool value; pointer value is being used instead}}
takes_bool(x); // no-warning
Same as cast.
https://reviews.llvm.org/D25731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
NoQ marked an inline comment as done.
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:149
BugReporter &BR) const {
MatchFinder F;
Callback CB(this, BR, AM.getAnalysisDeclContext(D));
alexshap wrote:
> probably it would make sense to move "MatchFinder F;" to the line 276 (closer
> to the place where it's actually being used)(or maybe i'm missing smth ?)
Yep, great idea.
Comment at: test/Analysis/number-object-conversion.c:14
+ if (p) {} // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
+ if (!p) {} // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
+ p ? 1 : 2; // expected-warning{{Converting 'CFNumberRef' to a plain boolean
value for branching; please compare the pointer to NULL instead to suppress
this warning}}
zaks.anna wrote:
> How about:
> "Converting 'CFNumberRef' pointer to a plain boolean value; instead, compare
> the pointer to NULL or compare to the encapsulated scalar value"
>
> - I've added "pointer".
> - I would remove "for branching". Does it add anything?
> - Also, we should remove "please" as it makes the warning text longer.
>
> I would remove "for branching". Does it add anything?
Because there's otherwise no obvious boolean value around, i wanted to point
out what exactly is going on.
> or compare to the encapsulated scalar value
They don't necessarily compare the value. Maybe "take"?
> I've added "pointer".
Not sure it's worth keeping for other objects ("`'NSNumber *' pointer`" sounds
like a pointer to a pointer).
https://reviews.llvm.org/D25731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
NoQ updated this revision to Diff 75411.
NoQ marked 5 inline comments as done.
NoQ added a comment.
Address review comments. Add the forgotten tests.
https://reviews.llvm.org/D25731
Files:
lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp
test/Analysis/number-object-conversion.c
test/Analysis/number-object-conversion.cpp
test/Analysis/number-object-conversion.m
Index: test/Analysis/number-object-conversion.m
===
--- test/Analysis/number-object-conversion.m
+++ test/Analysis/number-object-conversion.m
@@ -10,30 +10,36 @@
void bad(NSNumber *p) {
#ifdef PEDANTIC
- if (p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- if (!p) {} // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- (!p) ? 1 : 2; // expected-warning{{Converting 'NSNumber *' to a plain boolean value for branching; please compare the pointer to nil instead to suppress this warning}}
- (BOOL)p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; please compare the pointer to nil instead to suppress this warning}}
- if (p == 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; please compare the pointer to nil instead to suppress this warning}}
- if (p > 0) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+ if (p) {} // expected-warning{{Converting 'NSNumber *' pointer to a branch condition; instead, compare the pointer to nil or take the encapsulated scalar value}}
+ if (!p) {} // expected-warning{{Converting 'NSNumber *' pointer to a branch condition; instead, compare the pointer to nil or take the encapsulated scalar value}}
+ (!p) ? 1 : 2; // expected-warning{{Converting 'NSNumber *' pointer to a branch condition; instead, compare the pointer to nil or take the encapsulated scalar value}}
+ (BOOL)p; // expected-warning{{Converting 'NSNumber *' pointer to a plain BOOL value; instead, compare the pointer to nil or take the encapsulated scalar value}}
+ if (p == 0) {} // expected-warning{{Converting 'NSNumber *' pointer to a plain integer value; instead, compare the pointer to nil or take the encapsulated scalar value}}
+#else
+ if (p) {} // no-warning
+ if (!p) {} // no-warning
+ (!p) ? 1 : 2; // no-warning
+ (BOOL)p; // no-warning
+ if (p == 0) {} // no-warning
#endif
- if (p == YES) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- if (p == NO) {} // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- BOOL x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- x = p; // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- x = (p == YES); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- if (p == 1) {} // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- int y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- y = p; // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
- takes_boolean(p); // expected-warning{{Converting 'NSNumber *' to a plain BOOL value; pointer value is being used instead}}
- takes_integer(p); // expected-warning{{Converting 'NSNumber *' to a plain integer value; pointer value is being used instead}}
+ if (p > 0) {} // expected-warning{{Converting 'NSNumber *' pointer to a plain integer value; pointer value is being used instead}}
+ if (p == YES) {} // expected-warning{{Converting 'NSNumber *' pointer to a plain BOOL value; pointer value is being used instead}}
+ if (p == NO) {} // expected-warning{{Converting 'NSNumber *' pointer to a plain BOOL value; pointer value is being used instead}}
+ BOOL x = p; // expected-warning{{Converting 'NSNumber *' pointer to a plain BOOL value; pointer value is being used instead}}
+ x = p; // expected-warning{{Converting 'NSNumber *' pointer to a plain BOOL value; pointer value is being used instead}}
+ x = (p == YES); // expected-warning{{Converting 'NSNumber *' pointer to a plain BOOL value; pointer value is being used instead}}
+ if (p == 1) {} // expected-warning{{Converting 'NSNumber *' pointer to a plain integer value; pointer value is being used instead}}
+ int y = p; // expected-warning{{Converting 'NSNumber *' pointer to a plain integer value; pointer value is being used instead}}
+ y = p; // expected-warning{{Converting 'NSNumber *' pointer to a plain integer value; pointer value is being u
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
zaks.anna added inline comments. Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:111 + QualType ObjT = (IsCpp || IsObjC) + ? Obj->getType().getCanonicalType().getUnqualifiedType() + : Obj->getType(); NoQ wrote: > zaks.anna wrote: > > Why do we need a case split here? Would calling > > getCanonicalType().getUnqualifiedType() result in a wrong result for ObjC? > It'd result in a wrong result for `CFNumberRef`, which is a typedef we > shouldn't remove (turning this into `struct __CFNumber *` would be ugly). > > I'd probably rather avoid the case split by removing the > `.getCanonicalType()` part, because rarely anybody makes typedefs for > `NSNumber*` or `OSBoolean*` etc (such as in tests with the word "sugared"). > > Or i could descend down to the necessary typedef for C objects, discarding > all user's typedefs but not library typedefs. But that'd be even more > complicated and probably not very useful for such minor matter. > It'd result in a wrong result for CFNumberRef, which is a typedef we > shouldn't > remove (turning this into struct __CFNumber * would be ugly). I see. Displaying "CFNumberRef" is definitely the right thing to do! Please, add a comment to explain this. https://reviews.llvm.org/D25731 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
alexshap added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:149
BugReporter &BR) const {
MatchFinder F;
Callback CB(this, BR, AM.getAnalysisDeclContext(D));
probably it would make sense to move "MatchFinder F;" to the line 276 (closer
to the place where it's actually being used)(or maybe i'm missing smth ?)
https://reviews.llvm.org/D25731
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D25731: [analyzer] NumberObjectConversion: Support OSNumber and CFNumberRef.
NoQ added a comment. Ouch, i think i forgot about `OSNumber`, including tests. Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:111 + QualType ObjT = (IsCpp || IsObjC) + ? Obj->getType().getCanonicalType().getUnqualifiedType() + : Obj->getType(); zaks.anna wrote: > Why do we need a case split here? Would calling > getCanonicalType().getUnqualifiedType() result in a wrong result for ObjC? It'd result in a wrong result for `CFNumberRef`, which is a typedef we shouldn't remove (turning this into `struct __CFNumber *` would be ugly). I'd probably rather avoid the case split by removing the `.getCanonicalType()` part, because rarely anybody makes typedefs for `NSNumber*` or `OSBoolean*` etc (such as in tests with the word "sugared"). Or i could descend down to the necessary typedef for C objects, discarding all user's typedefs but not library typedefs. But that'd be even more complicated and probably not very useful for such minor matter. https://reviews.llvm.org/D25731 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
