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

[David Blaikie]

Hi Eli,

Thanks for the feedback - sorry for my delayed response.

On Thu, Sep 29, 2011 at 1:58 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> 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?

Looks like this is a bit trickier, actually, and the more I think
about it, the less I'm sure this warning should be implemented here at
all.

So it looks like there's a few problems. The general approach of the
current code is to diagnose any implicit conversion chain that starts
with GNUNull but doesn't involve an implicit cast to a pointer type
(this isn't an exact match for the language semantics I don't think -
integer zero literals /are/ null pointer literals, there is no
conversion at work in that case - though I don't know if that causes
any actual problems). In some cases that criteria isn't met.

1) static_cast<int*>(NULL) - there is no implicit conversion (I
believe there used to be up until a few weeks ago - when there was
some discussion on the list about removing these redundant implicit
casts & making the static_cast the cast that does the work (previously
the static_cast in this case would've been a no-op, just forcing the
implicit cast to be used on its argument))

2) the conditional operator does some strange things to its arguments
when checking implicit conversions there to account for the usual
arithmetic conversions causing the arguments to switch from unsigned
to signed & back again, as far as I can gather.

This might be too heavy handed, but I was wondering if we could treat
null pointer literals (0, 0L, false, etc) the same way that function
names are treated when using them as function pointers (this was
perhaps inspired by Lang's recent dabbling in the function pointer
conversion warning case he posted earlier today) - the type of a
function name is not known when it is named (due to overloading) &
context must be used to reduce it to a particular type. If we could do
something similar with zero integer constants (including GNUNull) then
we could detect when we were collapsing a zero integer constant to an
integer type instead of a pointer type & warn there instead.

I'm not sure how else we might handle this as it's just a poor fit in
this conversion testing code for a bunch of reasons.

[I'll post my updated diff with some failing tests (the static_cast
example above being one - and the conditional operator one I have a
clear example for (I'm just rebuilding right now so I can't show it
precisely) but I just can't remember exactly which way the test is
failing right now) when I validate the current state of the patch]

> 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.

Yeah - perhaps just getting this logic somewhere else would address
any concerns over the naming here.

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

Fair enough - I've removed that in my local copy, but the above
broader issues are still pending.

- David