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

[Ahmed Charles]

Testing short, int and long wouldn't detect this issue on windows 64,
because int and long are the same size. Using size_t seems like a good
option and so is adding all the other integer types.
------------------------------
From: David Blaikie
Sent: 9/28/2011 10:20 PM
To: llvm cfe
Subject: [cfe-commits] [PATCH] fix warn_impcast_null_pointer_to_integer for
pointer-sized integers

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

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

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.

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

[after this I'll add a fixit to replace NULL with the correct integer type
zero value]

Thanks for your time,
- David
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110929/6c6f41a1/attachment.html>