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

[Douglas Gregor]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110616/340464ed/attachment.html>