+patch
On Tue, Aug 7, 2012 at 3:50 PM, David Blaikie <[email protected]> wrote: > 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
null_pointers.diff
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
