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

[David Blaikie]

+correct patch

The diagnostic name is still "unusual-null-conversion" - could perhaps
do with some massaging.

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 --------------
A non-text attachment was scrubbed...
Name: null_pointers.diff
Type: application/octet-stream
Size: 10112 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120807/b307073b/attachment.obj>