[PATCH] D25731: [analyzer] NumberObjectConversion: Support CFNumberRef.

[Anna Zaks via cfe-commits]

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