[cfe-commits] Patch: Add a warning for NULL used in arithmetic operations
Eli Friedman
eli.friedman at gmail.com
Wed Jun 15 14:48:46 PDT 2011
On Wed, Jun 15, 2011 at 2:31 PM, Richard Trieu <rtrieu at google.com> wrote:
> 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.
Not so for ObjC++.
> 2) Should member pointers and block pointers behave the same as other
> pointers for relational and quality operators with NULL and nullptr?
Equality, yes. Relational operators with member and block pointers
aren't legal.
> 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?
Filed http://llvm.org/bugs/show_bug.cgi?id=10145 for the block
pointer+nullptr thing.
-Eli
More information about the cfe-commits
mailing list