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

[David Blaikie]

On Tue, Aug 7, 2012 at 7:44 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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.

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