Hi David, Can you split the Context == Unevaluated -> isUnevaluatedContext() refactoring out and commit that separately?
+ else if (!isa<IntegerLiteral>(FromSansParens) && + !isa<OpaqueValueExpr>(FromSansParens) && !isUnevaluatedContext()) 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. It would be nice to add a comment referencing core issue 903 somewhere in here. On Mon, Aug 6, 2012 at 2:52 PM, David Blaikie <[email protected]> wrote: > On Fri, Jul 27, 2012 at 4:04 PM, Sean Silva <[email protected]> wrote: > > + "initialization of pointer of type %0 to null from a non-trivial > zero " > > + "expression">, InGroup<UnusualNullConversion>; > > > > is this using a specific technical meaning for "non-trivial"? If not, > > it would probably be better to say "suspicious zero expression" or > > "unusual zero expression". > > Rephrased to: > > "initialization of pointer of type <foo> with an unusual null pointer > expression" Perhaps: warning: expression which evaluates to zero treated as a null pointer constant of type %0 > > + bool isUnevaluatedContext() const { > > + return ExprEvalContexts.back().Context == Sema::Unevaluated; > > + } > > > > Given the comment, maybe name it "isCurrentContextUnevaluated()"? > > Otherwise, when I see this called I ask myself "what is an unevaluated > > context?". > > You may ask this, but the C++ standard talks about "unevaluated > contexts" and so using the same terminology is hopefully helpful to > those dealing with these things between both standard and > implementation. While I agree the function could possibly be > rephrased, I hesitate to remove/reorder the words "unevaluated > context" from it - perhaps other reviews will have some > opinions/pointers here. I agree, this name is appropriate, since this directly represents a term from the C++ standard. > > More generally, there seem to be a lot of "bare" uses of > > ExprEvalContexts; it might pay to refactor this to be encapsulated a > > bit better so that the invariants can be asserted and documented (or > > at least put a FIXME above ExprEvalContexts). For example, it's a bit > > weird to see something like > > > > ExprEvalContexts.back().Context = > > ExprEvalContexts[ExprEvalContexts.size()-2].Context; > > - if (ExprEvalContexts.back().Context == Unevaluated) > > + if (isUnevaluatedContext()) > > > > the `isUnevaluatedContext()` call is "encapsulated", but the > > `ExprEvalContexts.back().Context = > > ExprEvalContexts[ExprEvalContexts.size()-2].Context;` is not, and so > > the connection between the two gets lost. I'm not entirely sure what a > > good name for this unencapsulated statement would be, but something > > like: > > > > ExprEvalContexts.copyCurrentContextFromEnclosing(); > > if (ExprEvalContexts.isCurrentContextUnevaluated()) > > return E; > > return TransformToPE(*this).TransformExpr(E); > > > > would make it a lot clearer what is going on, plus give nice places to > > document/assert invariants. It seems like most uses of > > ExprEvalContexts are either calls to .back(), so just getting a method > > named `.getCurrentContext()` would be a win. What do you think? Sorry > > that this is kind of tangential from your patch :) > > Yeah, it is rather tangential - happy enough to do it at some point > (would rather not include it in the same patch - but it could go > before or after) though. Again, perhaps other reviewers will weigh in > with some thoughts/preferences here. 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.
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
