[cfe-commits] [patch] warn on use of non-0/NULL/nullptr expressions in conditional operator operands and comparisons

[Richard Smith]

On Wed, Aug 15, 2012 at 2:36 PM, David Blaikie <dblaikie at gmail.com> wrote:

> Apparently our checks for weird null pointer constants (false, '\0',
> 3-3, non-type template parameters, non-literal const ints, etc) didn't
> actually catch cases where these null pointers were used in
> conditional operands (x ? ptr : false) or comparisons (cstr == '\0').
>
> This patch fixes that (& I've just updated two test cases to verify
> the two cases - conditional and comparison. I could add more to cover
> the various null pointer types too, but that doesn't seem to be very
> valuable) but has one rather questionable/wrong/discussion-worthy
> part, based on the way I've solved this. The solution was simply to
> move these checks from where they are to where they probably should
> be: SemaChecking.cpp
>
> The problem with SemaChecking's general approach is that it basically
> tries to find the maximally distinct types that don't involve any
> explicit constructs along the way (eg: find the target type through
> which a particular expression is ultimately used then compare that to
> the intrinsic type of the expression, ignoring all the implicit stuff
> in between - if the difference is interesting, warn). This includes
> SubstNonTypeTemplateParmExprs - it walks right through those. As long
> as it does that, this warning as I've written it won't catch this
> case:
>
> template<int X> void func() { int *i = X; }
> ...
> func<0>();
>
> But, honestly, I don't think I've found any instances of this apart
> from in LLVM's test suite.


This was the exact situation which originally motivated core issue 903 (the
change to remove non-literal-zero null pointer constants from C++), so I
think we should catch it.


> For the purposes of not regressing the test
> suite I tried a dirty hack (you'll see this in the attached patch)
> where "IgnoreParenImpCasts" doesn't ignore
> SubstNonTypeTemplateParmExprs - this maintains compatibility and
> doesn't break any tests... but I really expect it should (break some
> other tests) I just don't know what those tests are. This function is
> called from a variety of places, and I expect some of them might care
> (but perhaps they actually want the behavior change I've made - who
> knows)
>

This change seems extremely risky. I would be surprised if none of the
callers are relying on this... but it seems like it could be rather a lot
of work to find out.


> So I just thought I'd put this out there - chances are the
> simplest/low-risk thing to do is to revert my change to
> IgnoreParenImpCasts and modify the one test case that covers dependent
> null pointer expressions such as that (like I said, doesn't really
> catch any bugs that I've seen)


I don't see any value in having SemaChecking look through
SubstNonTypeTemplateParmExprs. Could you change its calls to
IgnoreParenImpCasts to use something else which doesn't skip them?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120815/26038b73/attachment.html>