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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
