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

[Sean Silva]

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

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

I would also assert `!ExprEvalContexts.empty()`, or document why it
can't be empty.

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

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