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

[Richard Smith]

Hi David,

Can you split the Context == Unevaluated -> isUnevaluatedContext()
refactoring out and commit that separately?

+      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.

It would be nice to add a comment referencing core issue 903 somewhere in
here.

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


> > +  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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120806/a7e5ef98/attachment.html>