On Tue, Aug 7, 2012 at 7:44 PM, Richard Smith <[email protected]> wrote: > 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.
Swapping one strawman for another I've renamed this "non-literal-null-conversion" - it's long & not entirely correct, but perhaps gets the point across better than "unusual". > Patch LGTM otherwise. Committed as r161501. Thanks! - David > >> >> 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
