Hi Eli, Thanks for the feedback - sorry for my delayed response.
On Thu, Sep 29, 2011 at 1:58 PM, Eli Friedman <[email protected]> wrote: > On Wed, Sep 28, 2011 at 10:18 PM, David Blaikie <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
