<div class="gmail_quote">On Tue, Jun 14, 2011 at 7:10 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word"><div><div></div><div class="h5"><br><div><div>On Jun 13, 2011, at 5:00 PM, Richard Trieu wrote:</div><br><blockquote type="cite"><br><br><div class="gmail_quote">On Thu, May 26, 2011 at 3:10 AM, Chandler Carruth <span dir="ltr"><<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote"><div>On Mon, May 2, 2011 at 5:48 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br></div><div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span style="font-family:Verdana, Arial, Helvetica, sans-serif;font-size:12px"><pre style="font-size:12px"><span>Add warning when NULL constant value is used improperly in expressions.</span></pre></span></blockquote><div>
<br></div></div><div>Some high-level comments:</div><div><br></div><div>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.</div>
</div></blockquote><div>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. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="gmail_quote">
<div><br></div><div>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?</div>
</div></blockquote><div>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.</div>
<div><br></div><div>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.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="gmail_quote">
<div><br></div><div>3) Because these are warnings, we can't return ExprError; that changes the parse result.</div></div>
</blockquote></div>Removed the early exit with ExprError. </blockquote><br></div><div><br></div></div></div><div><div>+ bool LeftNull = Expr::NPCK_GNUNull ==</div><div>+ lhs.get()->isNullPointerConstant(Context,</div>
<div>+ Expr::NPC_ValueDependentIsNotNull);</div><div>+ bool RightNull = Expr::NPCK_GNUNull ==</div><div>+ rhs.get()->isNullPointerConstant(Context,</div><div>+ Expr::NPC_ValueDependentIsNotNull);</div>
<div>+</div><div>+ // Detect when a NULL constant is used improperly in an expression.</div><div>+ if (LeftNull || RightNull) {</div><div><br></div><div>I think you want: </div><div><br></div><div><span style="white-space:pre-wrap"> </span>if (LeftNull != RightNull) {</div>
</div></div></blockquote><div><br></div><div>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.</div>
<meta http-equiv="content-type" content="text/html; charset=utf-8"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"><div style="word-wrap:break-word"><div><div><br></div>
<div>here? At the very least, we shouldn't emit two warnings when both sides of the operator are NULL.</div><div><br></div><div><div>+ // These are the operations that would not make sense with a null pointer</div>
<div>+ // if the other expression the other expression is not a pointer.</div><div>+ if ((LeftNull != RightNull) && !lhs.get()->getType()->isPointerType() &&</div><div>+ !rhs.get()->getType()->isPointerType()) {</div>
<div><br></div><div>You also need to check for member pointers, block pointers, and Objective-C pointers here.</div><div><br></div><div><span style="white-space:pre-wrap"> </span>- Doug</div></div></div></div></blockquote>
</div><br><div>Two points here.</div><div>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.</div>
<div>2) Should member pointers and block pointers behave the same as other pointers for relational and quality operators with NULL and nullptr? 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?</div>