<div class="gmail_quote">On Wed, Aug 15, 2012 at 2:36 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Apparently our checks for weird null pointer constants (false, '\0',<br>
3-3, non-type template parameters, non-literal const ints, etc) didn't<br>
actually catch cases where these null pointers were used in<br>
conditional operands (x ? ptr : false) or comparisons (cstr == '\0').<br>
<br>
This patch fixes that (& I've just updated two test cases to verify<br>
the two cases - conditional and comparison. I could add more to cover<br>
the various null pointer types too, but that doesn't seem to be very<br>
valuable) but has one rather questionable/wrong/discussion-worthy<br>
part, based on the way I've solved this. The solution was simply to<br>
move these checks from where they are to where they probably should<br>
be: SemaChecking.cpp<br>
<br>
The problem with SemaChecking's general approach is that it basically<br>
tries to find the maximally distinct types that don't involve any<br>
explicit constructs along the way (eg: find the target type through<br>
which a particular expression is ultimately used then compare that to<br>
the intrinsic type of the expression, ignoring all the implicit stuff<br>
in between - if the difference is interesting, warn). This includes<br>
SubstNonTypeTemplateParmExprs - it walks right through those. As long<br>
as it does that, this warning as I've written it won't catch this<br>
case:<br>
<br>
template<int X> void func() { int *i = X; }<br>
...<br>
func<0>();<br>
<br>
But, honestly, I don't think I've found any instances of this apart<br>
from in LLVM's test suite.</blockquote><div><br></div><div>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.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">For the purposes of not regressing the test<br>
suite I tried a dirty hack (you'll see this in the attached patch)<br>
where "IgnoreParenImpCasts" doesn't ignore<br>
SubstNonTypeTemplateParmExprs - this maintains compatibility and<br>
doesn't break any tests... but I really expect it should (break some<br>
other tests) I just don't know what those tests are. This function is<br>
called from a variety of places, and I expect some of them might care<br>
(but perhaps they actually want the behavior change I've made - who<br>
knows)<br></blockquote><div><br></div><div>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.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
So I just thought I'd put this out there - chances are the<br>
simplest/low-risk thing to do is to revert my change to<br>
IgnoreParenImpCasts and modify the one test case that covers dependent<br>
null pointer expressions such as that (like I said, doesn't really<br>
catch any bugs that I've seen)</blockquote><div><br></div><div>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?</div>
</div>