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

[David Blaikie]

On Wed, Aug 15, 2012 at 2:56 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> 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.

Agreed - sorry, should've been more clear. This was presented mostly
as a straw man to demonstrate the root cause of the problem, I realize
it's not a terribly practical solution.

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

As we discussed in person (repeating here for posterity/public record
- correct/clarify/expound on this if you wish) there are some
diagnostics that gain at least some value from being able to look
through these Exprs at least in some (possibly all) parts of
SemaChecking.

Now that I write this I'm not entirely sure I agree (perhaps I'm
forgetting context from our discussion on Friday) with what I thought
we'd decided. I know at one point we thought there was one caller we
could teach not to walk through the SubstNonTypeTemplateParmExprs &
that would suffice.

Did we have any particularly compelling examples of when we /should/
walk through them? It seems like we want to treat them the same way we
would an opaque boundary like a function call.

> Could you change its calls to
> IgnoreParenImpCasts to use something else which doesn't skip them?

Possibly some of the calls in SemaChecking should be, yes.
Theoretically (since I can't think of the concrete examples we may've
discussed on Friday) some diagnostics might benefit from looking
through SubstNonTypeTemplateParmExprs and some might be hurt by doing
so - if that's the case, this gets hairier. Do we do both? (try it
with and without and then we can filter diagnostics on/off based on
whether the root (or leaf) in question is a
SubstNonTypeTemplateParmExpr)

- David