Hi David,<div><br></div><div>Can you split the Context == Unevaluated -> isUnevaluatedContext() refactoring out and commit that separately?</div><div><br></div><div><div>+      else if (!isa<IntegerLiteral>(FromSansParens) &&</div>
<div>+               !isa<OpaqueValueExpr>(FromSansParens) && !isUnevaluatedContext())</div><div><br></div><div>Could you split out another NPCK_ value instead of these checks? I'm concerned about the constructs which Expr::isNullPointerConstant skips past but this check doesn't.</div>
<div><br></div><div>It would be nice to add a comment referencing core issue 903 somewhere in here.</div><br><div class="gmail_quote">On Mon, Aug 6, 2012 at 2:52 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"><div class="im">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 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>
</div>Rephrased to:<br>
<br>
"initialization of pointer of type <foo> with an unusual null pointer<br>
expression"</blockquote><div><br></div><div>Perhaps:</div><div><br></div><div>warning: expression which evaluates to zero treated as a null pointer constant of type %0</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">
> +  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>
</div>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.</blockquote><div><br></div><div>I agree, this name is appropriate, since this directly represents a term from the C++ standard.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im">> More generally, there seem to be a lot of "bare" uses of</div><div class="im">
> 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>
</div>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.</blockquote><div><br></div><div>I think, since this function is directly manipulating ExprEvalContexts anyway, it would be better to leave it as-is for now. The current half-way change makes the code less clear. With that change, the isUnevaluatedContext() refactoring LGTM.</div>
</div></div>