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

[David Blaikie]

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"

> +  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 would also assert `!ExprEvalContexts.empty()`, or document why it
> can't be empty.

Added an assertion.

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

- David

>
> --Sean Silva
>
> On Fri, Jul 27, 2012 at 1:57 PM, David Blaikie <dblaikie at gmail.com> wrote:
>> While we currently warn on code like this:
>>
>>   bool *b = false; // or any other pointer type, for that matter, but
>> bool makes it clear how someone might accidentally write this
>>
>> We don't warn on code like this
>>
>>   char *c = '\0'; // again, applicable to any pointer type
>>
>> So I figured I would generalize the former warning & use a whitelist
>> rather than a blacklist, only allowing expressions of nullptr_t type
>> or literal zeros of integer type (not char type). This catches a few
>> odd null pointer constructs in the test suite and in the wild
>> including things like this:
>>
>>   template<typename T> T *foo() { return T(); } // found in the test
>> suite, valid for any integral type T, but a tad surprising
>>
>> & a lot of things like
>>
>>   strtol(x, '\0', base)
>>
>> plus a few more subtle things.
>>
>> In the interests of not breaking all over gunit, the warning is
>> suppressed for unevaluated contexts (gunit does some sfinae fun to see
>> if certain expressions are valid null pointers) & refactored the
>> unevaluated context check across all the current callers.
>>
>> This helps pave the way for codebases to migrate for a DR Richard
>> Smith mentioned to me that would actually make this an actual standard
>> requirement rather than just a warning (the gunit case would no longer
>> be a problem then because the SFINAE would find it as an error & take
>> a different path anyway).
>>
>> Thoughts? Naming & diagnostic text are certainly open to improvements...
>>
>> PS: you'll see we're actually missing some cases of this (test case
>> TODO in the patch) that even apply to the bool case we thought we were
>> catching. I'll work on fixing that hole next (probably move this check
>> to SemaChecking where it presumably belongs).
>>
>> - David
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: null_pointers.diff
Type: application/octet-stream
Size: 11987 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120806/ba33c524/attachment.obj>