Hi Richard, > + 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.
Makes a lot of sense. I've implemented an attempt at this - separating out NPCK_ZeroInteger into NPCK_ZeroLiteral and NPCK_ZeroExpression. > It would be nice to add a comment referencing core issue 903 somewhere in > here. Mentioned core issue 903 in the definition of NPCK_ZeroExpression. > 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 Reworded as suggested. >> > + 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. Committed as r161355. Thanks, - David _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
