+correct patch The diagnostic name is still "unusual-null-conversion" - could perhaps do with some massaging.
On Tue, Aug 7, 2012 at 3:50 PM, David Blaikie <[email protected]> wrote: > +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
