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

Richard Trieu rtrieu at google.com
Thu Jun 16 14:45:32 PDT 2011


On Thu, Jun 16, 2011 at 11:03 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On Jun 15, 2011, at 4:39 PM, Richard Trieu wrote:
>
>
>
> On Wed, Jun 15, 2011 at 2:48 PM, Eli Friedman <eli.friedman at gmail.com>wrote:
>
>> 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
>>
>
> Thanks for the help, Eli.  Added the check for member, block, and
> Objective-C pointers and tests for them.  Fixed cases with two NULL's so
> they only produce one warning instead of two.
>
>
>
> Index: test/SemaObjCXX/null_objc_pointer.mm
> ===================================================================
> --- test/SemaObjCXX/null_objc_pointer.mm (revision 0)
> +++ test/SemaObjCXX/null_objc_pointer.mm (revision 0)
> @@ -0,0 +1,13 @@
> +// RUN: %clang_cc1 -fsyntax-only -verify %s
> +#import <stddef.h>
> +
>
> We generally try not to import headers in our testsuite, because we want it
> to be self-contained and avoid platform dependencies. I suggest just
> #define'ing NULL appropriately so that the tests will run the same way
> regardless of the platform's definition of NULL.
>
> Aside from that, it looks good. Thanks!
>
> - Doug
>
> Removed the import and used a definition for NULL instead.  Also added back
the test for block pointer with nullptr, since that was recently fixed.
 Filed at revision 133196.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110616/1014bd4b/attachment.html>


More information about the cfe-commits mailing list