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

[Artem Dergachev via cfe-commits]

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