[cfe-commits] [PATCH] fix warn_impcast_null_pointer_to_integer for pointer-sized integers

[Eli Friedman]

On Wed, Sep 28, 2011 at 10:18 PM, David Blaikie <dblaikie at gmail.com> wrote:
> warn_impcast_null_pointer_to_integer (" implicit conversion of NULL constant
> to integer"), a warning that is meant to trigger whenever NULL is used in a
> non-pointer context, was not firing whenever the integer type it was
> converted to was the same size as the pointer on the given platform.
> A __null expression was of type 'int' on 32 bit platforms and 'long' on 64
> bit platforms, etc. This meant that there was no conversion between __null
> and int/long, as they were the exact same type. So the check for
> warn_impcast_null_pointer_to_integer never fired, since it happened
> afterwards.
> To fix this I moved the check up to the top of the main
> CheckImplicitConversions function & had to remove a shortcut check in one of
> the callers (this was the check for E.getType() == T that was causing the
> warning to be missed, though just removing that wouldn't've been sufficient
> since there was another similar check at the start of
> CheckImplicitConversions).

There are multiple callers of CheckImplicitConversion; it looks like
CheckConditionalOperator/CheckConditionalOperand need changes as well?

Also, it might be nice if you could come up with a better name than
CheckImplicitConversion; I can't think of anything off the top of my
head, though.

> I've updated the test case only slightly to add one case that would've
> failed without this change "long x = NULL;" (the conversions.cpp only
> compiles as 64 bit, and since all the tests for NULL conversion were using
> int they never hit this bug. I'm not sure if it's worth testing all the
> cases already covered but in a 32 bit build too - maybe at least a quick
> sanity check might be nice (my own personal test case was to use short, int,
> and long initialized with NULL, assigned with NULL, then a function that
> accepted all 3 types that was passed NULL to all of them))

Doesn't really matter.

> This solution might be a bit ham-fisted as I'm not sure which operations are
> the cheapest - between type comparisons, type canonicalization, etc. There's
> a semi-redundant check for E->getType() == T - a check, then a type
> canonicalization, then a check on the canonicalized type. If canonicalizing
> the types is cheap enough, the check before canonicalization could be
> removed.

getCanonicalType() is just a load plus a bit of arithmetic; the extra
check isn't necessary.

> I also simplified the GNUNull check, not just for performance but
> correctness. In one particular case (reinterpret_cast<int&&>('a')) trying to
> call isNullPointerConstant on that whole cast Expression crashes because it
> cannot be evaluated (perhaps this should be corrected, but it wasn't
> necessary for now).

Filed http://llvm.org/bugs/show_bug.cgi?id=11040 .  Definitely
something we should fix at some point anyway.

-Eli