On Tue, Aug 7, 2012 at 4:49 PM, David Blaikie <[email protected]> wrote:
> +correct patch > > The diagnostic name is still "unusual-null-conversion" - could perhaps > do with some massaging. Yes, I agree, but I can't think of anything which is much better. Patch LGTM otherwise. > 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 >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
