On Thu, Jun 16, 2011 at 11:03 AM, Douglas Gregor <[email protected]> wrote:
> > On Jun 15, 2011, at 4:39 PM, Richard Trieu wrote: > > > > On Wed, Jun 15, 2011 at 2:48 PM, Eli Friedman <[email protected]>wrote: > >> On Wed, Jun 15, 2011 at 2:31 PM, Richard Trieu <[email protected]> wrote: >> > On Tue, Jun 14, 2011 at 7:10 AM, Douglas Gregor <[email protected]> >> wrote: >> >> >> >> On Jun 13, 2011, at 5:00 PM, Richard Trieu wrote: >> >> >> >> >> >> On Thu, May 26, 2011 at 3:10 AM, Chandler Carruth < >> [email protected]> >> >> wrote: >> >>> >> >>> On Mon, May 2, 2011 at 5:48 PM, Richard Trieu <[email protected]> >> 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
