<div class="gmail_quote">On Tue, Aug 7, 2012 at 4:49 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+correct patch<br>
<br>
The diagnostic name is still "unusual-null-conversion" - could perhaps<br>
do with some massaging.</blockquote><div><br></div><div>Yes, I agree, but I can't think of anything which is much better. Patch LGTM otherwise. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5">
On Tue, Aug 7, 2012 at 3:50 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> +patch<br>
><br>
> On Tue, Aug 7, 2012 at 3:50 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> Hi Richard,<br>
>><br>
>>> +      else if (!isa<IntegerLiteral>(FromSansParens) &&<br>
>>> +               !isa<OpaqueValueExpr>(FromSansParens) &&<br>
>>> !isUnevaluatedContext())<br>
>>><br>
>>> Could you split out another NPCK_ value instead of these checks? I'm<br>
>>> concerned about the constructs which Expr::isNullPointerConstant skips past<br>
>>> but this check doesn't.<br>
>><br>
>> Makes a lot of sense. I've implemented an attempt at this - separating<br>
>> out NPCK_ZeroInteger into NPCK_ZeroLiteral and NPCK_ZeroExpression.<br>
>><br>
>>> It would be nice to add a comment referencing core issue 903 somewhere in<br>
>>> here.<br>
>><br>
>> Mentioned core issue 903 in the definition of NPCK_ZeroExpression.<br>
>><br>
>>> On Mon, Aug 6, 2012 at 2:52 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>>><br>
>>>> On Fri, Jul 27, 2012 at 4:04 PM, Sean Silva <<a href="mailto:silvas@purdue.edu">silvas@purdue.edu</a>> wrote:<br>
>>>> > +    "initialization of pointer of type %0 to null from a non-trivial<br>
>>>> > zero "<br>
>>>> > +    "expression">, InGroup<UnusualNullConversion>;<br>
>>>> ><br>
>>>> > is this using a specific technical meaning for "non-trivial"? If not,<br>
>>>> > it would probably be better to say "suspicious zero expression" or<br>
>>>> > "unusual zero expression".<br>
>>>><br>
>>>> Rephrased to:<br>
>>>><br>
>>>> "initialization of pointer of type <foo> with an unusual null pointer<br>
>>>> expression"<br>
>>><br>
>>><br>
>>> Perhaps:<br>
>>><br>
>>> warning: expression which evaluates to zero treated as a null pointer<br>
>>> constant of type %0<br>
>><br>
>> Reworded as suggested.<br>
>><br>
>>>> > +  bool isUnevaluatedContext() const {<br>
>>>> > +    return ExprEvalContexts.back().Context == Sema::Unevaluated;<br>
>>>> > +  }<br>
>>>> ><br>
>>>> > Given the comment, maybe name it "isCurrentContextUnevaluated()"?<br>
>>>> > Otherwise, when I see this called I ask myself "what is an unevaluated<br>
>>>> > context?".<br>
>>>><br>
>>>> You may ask this, but the C++ standard talks about "unevaluated<br>
>>>> contexts" and so using the same terminology is hopefully helpful to<br>
>>>> those dealing with these things between both standard and<br>
>>>> implementation. While I agree the function could possibly be<br>
>>>> rephrased, I hesitate to remove/reorder the words "unevaluated<br>
>>>> context" from it - perhaps other reviews will have some<br>
>>>> opinions/pointers here.<br>
>>><br>
>>><br>
>>> I agree, this name is appropriate, since this directly represents a term<br>
>>> from the C++ standard.<br>
>>><br>
>>>><br>
>>>> > More generally, there seem to be a lot of "bare" uses of<br>
>>>> > ExprEvalContexts; it might pay to refactor this to be encapsulated a<br>
>>>> > bit better so that the invariants can be asserted and documented (or<br>
>>>> > at least put a FIXME above ExprEvalContexts). For example, it's a bit<br>
>>>> > weird to see something like<br>
>>>> ><br>
>>>> >    ExprEvalContexts.back().Context =<br>
>>>> >        ExprEvalContexts[ExprEvalContexts.size()-2].Context;<br>
>>>> > -  if (ExprEvalContexts.back().Context == Unevaluated)<br>
>>>> > +  if (isUnevaluatedContext())<br>
>>>> ><br>
>>>> > the `isUnevaluatedContext()` call is "encapsulated", but the<br>
>>>> > `ExprEvalContexts.back().Context =<br>
>>>> > ExprEvalContexts[ExprEvalContexts.size()-2].Context;` is not, and so<br>
>>>> > the connection between the two gets lost. I'm not entirely sure what a<br>
>>>> > good name for this unencapsulated statement would be, but something<br>
>>>> > like:<br>
>>>> ><br>
>>>> > ExprEvalContexts.copyCurrentContextFromEnclosing();<br>
>>>> > if (ExprEvalContexts.isCurrentContextUnevaluated())<br>
>>>> >   return E;<br>
>>>> > return TransformToPE(*this).TransformExpr(E);<br>
>>>> ><br>
>>>> > would make it a lot clearer what is going on, plus give nice places to<br>
>>>> > document/assert invariants. It seems like most uses of<br>
>>>> > ExprEvalContexts are either calls to .back(), so just getting a method<br>
>>>> > named `.getCurrentContext()` would be a win. What do you think? Sorry<br>
>>>> > that this is kind of tangential from your patch :)<br>
>>>><br>
>>>> Yeah, it is rather tangential - happy enough to do it at some point<br>
>>>> (would rather not include it in the same patch - but it could go<br>
>>>> before or after) though. Again, perhaps other reviewers will weigh in<br>
>>>> with some thoughts/preferences here.<br>
>>><br>
>>><br>
>>> I think, since this function is directly manipulating ExprEvalContexts<br>
>>> anyway, it would be better to leave it as-is for now. The current half-way<br>
>>> change makes the code less clear. With that change, the<br>
>>> isUnevaluatedContext() refactoring LGTM.<br>
>><br>
>> Committed as r161355.<br>
>><br>
>> Thanks,<br>
>> - David<br>
</div></div></blockquote></div><br>