<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Jun 15, 2011, at 4:39 PM, Richard Trieu wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><br><br><div class="gmail_quote">On Wed, Jun 15, 2011 at 2:48 PM, Eli Friedman <span dir="ltr"><<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div><div></div><div class="h5">On Wed, Jun 15, 2011 at 2:31 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
> On Tue, Jun 14, 2011 at 7:10 AM, Douglas Gregor <<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>> wrote:<br>
>><br>
>> On Jun 13, 2011, at 5:00 PM, Richard Trieu wrote:<br>
>><br>
>><br>
>> On Thu, May 26, 2011 at 3:10 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>><br>
>> wrote:<br>
>>><br>
>>> On Mon, May 2, 2011 at 5:48 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>
>>>><br>
>>>> Add warning when NULL constant value is used improperly in expressions.<br>
>>><br>
>>> Some high-level comments:<br>
>>> 1) Why only handle GNUNull null-pointer-constants? I think there is a<br>
>>> good reason here (making them behave as much like nullptr as possible) but<br>
>>> it would be good to document that in comments at least.<br>
>><br>
>> The other kinds of nulls are zero integer and c++0x nullptr.<br>
>> Integral zero is valid for these operations. nullptr is a different type<br>
>> which already produces an error in these cases.<br>
>>><br>
>>> 2) What drives the set of invalid operations? I assume its those that<br>
>>> nullptr would be invalid for? If so, can you provide standard citations<br>
>>> against the draft standard? Also, is there no way to just turn on the C++0x<br>
>>> errors for nullptr when we see the GNUNull in C++03, but down-grade them to<br>
>>> warnings?<br>
>><br>
>> These operations are the ones where the null pointer would be used as an<br>
>> integer instead of a pointer. I've also copied the test, but using nullptr<br>
>> instead to show that they error in same places.<br>
>> It should be possible to change the NULL into a nullptr and then run the<br>
>> checks, but that would probably involve touching code in all the of<br>
>> operation checking functions. I feel that it would be better to keep this<br>
>> code in one place instead of spread across so many functions.<br>
>>><br>
>>> 3) Because these are warnings, we can't return ExprError; that changes<br>
>>> the parse result.<br>
>><br>
>> Removed the early exit with ExprError.<br>
>><br>
>> + bool LeftNull = Expr::NPCK_GNUNull ==<br>
>> + lhs.get()->isNullPointerConstant(Context,<br>
>> +<br>
>> Expr::NPC_ValueDependentIsNotNull);<br>
>> + bool RightNull = Expr::NPCK_GNUNull ==<br>
>> + rhs.get()->isNullPointerConstant(Context,<br>
>> +<br>
>> Expr::NPC_ValueDependentIsNotNull);<br>
>> +<br>
>> + // Detect when a NULL constant is used improperly in an expression.<br>
>> + if (LeftNull || RightNull) {<br>
>> I think you want:<br>
>> if (LeftNull != RightNull) {<br>
><br>
> No, I did want (LeftNull || RightNull) since something like (NULL * NULL)<br>
> should be warned on. I will add the check so that using two NULLs will only<br>
> produce one warning. The inequality check is already present for comparison<br>
> operators.<br>
>><br>
>> here? At the very least, we shouldn't emit two warnings when both sides of<br>
>> the operator are NULL.<br>
>> + // These are the operations that would not make sense with a null<br>
>> pointer<br>
>> + // if the other expression the other expression is not a pointer.<br>
>> + if ((LeftNull != RightNull) &&<br>
>> !lhs.get()->getType()->isPointerType() &&<br>
>> + !rhs.get()->getType()->isPointerType()) {<br>
>> You also need to check for member pointers, block pointers, and<br>
>> Objective-C pointers here.<br>
>> - Doug<br>
><br>
> Two points here.<br>
> 1) In Objective-C test, I see that NULL is defined as (void*)0 which will<br>
> not cause it to be picked up by the NULL checks for GNU null. There are<br>
> already warnings for improper conversion to integer so having a special case<br>
> for Objective-C is not required.<br>
<br>
</div></div>Not so for ObjC++.<br>
<div class="im"><br>
> 2) Should member pointers and block pointers behave the same as other<br>
> pointers for relational and quality operators with NULL and nullptr?<br>
<br>
</div>Equality, yes. Relational operators with member and block pointers<br>
aren't legal.<br>
<div class="im"><br>
> For<br>
> NULL, the relational operators give an invalid operand error. For nullptr,<br>
> relational operators for both pointers and equality operators for block<br>
> pointers give an invalid operand error. This is different from other<br>
> pointers which will not complain. Is this proper behavior for these type of<br>
> pointers?<br>
<br>
</div>Filed <a href="http://llvm.org/bugs/show_bug.cgi?id=10145" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=10145</a> for the block<br>
pointer+nullptr thing.<br>
<font color="#888888"><br>
-Eli<br>
</font></blockquote></div><br><div>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.</div></blockquote><div><br></div><div><br></div><div>Index: test/SemaObjCXX/null_objc_pointer.mm</div><div>===================================================================</div><div>--- test/SemaObjCXX/null_objc_pointer.mm<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div><div>+++ test/SemaObjCXX/null_objc_pointer.mm<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 0)</div><div>@@ -0,0 +1,13 @@</div><div>+// RUN: %clang_cc1 -fsyntax-only -verify %s</div><div>+#import <stddef.h></div><div>+</div><div><br></div><div>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.</div><div><br></div><div>Aside from that, it looks good. Thanks!</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div><div><br></div></div></body></html>