[cfe-commits] [patch] Warn on strange null pointers

[Richard Smith]

On Tue, Aug 7, 2012 at 4:49 PM, David Blaikie <dblaikie at gmail.com> 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 <dblaikie at gmail.com> wrote:
> > +patch
> >
> > On Tue, Aug 7, 2012 at 3:50 PM, David Blaikie <dblaikie at gmail.com>
> 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 <dblaikie at gmail.com>
> wrote:
> >>>>
> >>>> On Fri, Jul 27, 2012 at 4:04 PM, Sean Silva <silvas at purdue.edu>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120807/1c7fd947/attachment.html>