zaks.anna added inline comments.
================
Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:72
assert(Conv);
- const Expr *Osboolean = Result.Nodes.getNodeAs<Expr>("osboolean");
- const Expr *Nsnumber = Result.Nodes.getNodeAs<Expr>("nsnumber");
- bool IsObjC = (bool)Nsnumber;
- const Expr *Obj = IsObjC ? Nsnumber : Osboolean;
+ const Expr *CObject = Result.Nodes.getNodeAs<Expr>("c_object");
+ const Expr *CppObject = Result.Nodes.getNodeAs<Expr>("cpp_object");
----------------
I'd keep the names more specific. By reading this code alone it looks like you
are just checking for **any** ObjC object.
================
Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:111
+ QualType ObjT = (IsCpp || IsObjC)
+ ? Obj->getType().getCanonicalType().getUnqualifiedType()
+ : Obj->getType();
----------------
Why do we need a case split here? Would calling
getCanonicalType().getUnqualifiedType() result in a wrong result for ObjC?
================
Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:129
+ } else if (IsCpp) {
OS << "; please compare the pointer to NULL or nullptr instead "
"to suppress this warning";
----------------
Let's just recommend `nullptr` for C++, but allow both `NULL` and `nullptr`.
================
Comment at: lib/StaticAnalyzer/Checkers/NumberObjectConversionChecker.cpp:152
- auto OSBooleanExprM =
+ auto SuspiciousCExprM =
+ expr(ignoringParenImpCasts(
----------------
Please, use more a expressive name. I'd prefer a super long name if it's more
expressive.
================
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}}
----------------
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.
https://reviews.llvm.org/D25731
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
