[cfe-commits] Patch: Add a warning for NULL used in arithmetic operations

[Richard Trieu]

On Tue, Jun 14, 2011 at 7:10 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Jun 13, 2011, at 5:00 PM, Richard Trieu wrote:
>
>
>
> On Thu, May 26, 2011 at 3:10 AM, Chandler Carruth <chandlerc at google.com>wrote:
>
>> On Mon, May 2, 2011 at 5:48 PM, Richard Trieu <rtrieu at google.com> wrote:
>>
>>> Add warning when NULL constant value is used improperly in expressions.
>>>
>>>
>> Some high-level comments:
>>
>> 1) Why only handle GNUNull null-pointer-constants? I think there is a good
>> reason here (making them behave as much like nullptr as possible) but it
>> would be good to document that in comments at least.
>>
> The other kinds of nulls are zero integer and c++0x nullptr.  Integral zero
> is valid for these operations.  nullptr is a different type which already
> produces an error in these cases.
>
>>
>> 2) What drives the set of invalid operations? I assume its those that
>> nullptr would be invalid for? If so, can you provide standard citations
>> against the draft standard? Also, is there no way to just turn on the C++0x
>> errors for nullptr when we see the GNUNull in C++03, but down-grade them to
>> warnings?
>>
> These operations are the ones where the null pointer would be used as an
> integer instead of a pointer. I've also copied the test, but using nullptr
> instead to show that they error in same places.
>
> It should be possible to change the NULL into a nullptr and then run the
> checks, but that would probably involve touching code in all the of
> operation checking functions.  I feel that it would be better to keep this
> code in one place instead of spread across so many functions.
>
>>
>> 3) Because these are warnings, we can't return ExprError; that changes the
>> parse result.
>>
> Removed the early exit with ExprError.
>
>
>
> +  bool LeftNull = Expr::NPCK_GNUNull ==
> +      lhs.get()->isNullPointerConstant(Context,
> +                                       Expr::NPC_ValueDependentIsNotNull);
> +  bool RightNull = Expr::NPCK_GNUNull ==
> +      rhs.get()->isNullPointerConstant(Context,
> +                                       Expr::NPC_ValueDependentIsNotNull);
> +
> +  // Detect when a NULL constant is used improperly in an expression.
> +  if (LeftNull || RightNull) {
>
> I think you want:
>
> if (LeftNull != RightNull) {
>

No, I did want (LeftNull || RightNull) since something like (NULL * NULL)
should be warned on.  I will add the check so that using two NULLs will only
produce one warning.  The inequality check is already present for comparison
operators.

>
> here? At the very least, we shouldn't emit two warnings when both sides of
> the operator are NULL.
>
> +      // These are the operations that would not make sense with a null
> pointer
> +      // if the other expression the other expression is not a pointer.
> +      if ((LeftNull != RightNull) &&
> !lhs.get()->getType()->isPointerType() &&
> +          !rhs.get()->getType()->isPointerType()) {
>
> You also need to check for member pointers, block pointers, and Objective-C
> pointers here.
>
> - Doug
>

Two points here.
1) In Objective-C test, I see that NULL is defined as (void*)0 which will
not cause it to be picked up by the NULL checks for GNU null.  There are
already warnings for improper conversion to integer so having a special case
for Objective-C is not required.
2) Should member pointers and block pointers behave the same as other
pointers for relational and quality operators with NULL and nullptr?  For
NULL, the relational operators give an invalid operand error.  For nullptr,
relational operators for both pointers and equality operators for block
pointers give an invalid operand error.  This is different from other
pointers which will not complain.  Is this proper behavior for these type of
pointers?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110615/e7ac15e5/attachment.html>