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

[David Blaikie]

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