<div dir="ltr">On Wed, Oct 9, 2013 at 4:19 PM, Arthur O'Dwyer <span dir="ltr"><<a href="mailto:arthur.j.odwyer@gmail.com" target="_blank">arthur.j.odwyer@gmail.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Wed, Oct 9, 2013 at 3:57 PM, Richard Trieu <<a href="mailto:rtrieu@google.com">rtrieu@google.com</a>> wrote:<br>

> On Wed, Oct 9, 2013 at 2:53 PM, Arthur O'Dwyer <<a href="mailto:arthur.j.odwyer@gmail.com">arthur.j.odwyer@gmail.com</a>><br>
> wrote:<br>
>><br>
>> Well, I do think that you should run this diagnostic over a few real<br>
>> codebases<br>
><br>
> Already done.  My comments with the patch reflect the results of running<br>
> this warning over Google code.<br>
</div>[...]<br>
<div class="im">>  (B) where this diagnostic gave false positives on non-template code,<br>
> Nope, didn't find any.<br>
<br>
</div>Didn't you find some related to infinite loops? Or else why would you<br>
put in the special case to disable the warning on<br>
<br>
    void foo() { while (true) { ... foo(); ... } }<br>
<div class="im"><br>
?<br></div></blockquote><div>The first version of the warning didn't propagate values all the way to the exit CFG block thus caught every infinite loop.  Later versions required at least one path to the exit block.  The warning never caught this case and is only provided to show the limits of the warning. </div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
<br>
>  (C) where this diagnostic gave false positives on template code, and<br>
> // Same example as before.<br>
> template <int value><br>
> int sum() {<br>
>   return value + sum<value/2>;<br>
<br>
</div>Yeah, but this example is syntactically incorrect *and* even if the<br>
syntax errors are fixed it's just a verbose and<br>
buggy-on-negative-numbers substitute for __builtin_popcount(x-y) *and*<br>
I've given you the trivial fix — so I discount this example. :)<br>
<div class="im"><br></div></blockquote><div>Test cases are reduced from real code, since not all code is available or freely share-able.  The original code had more integer parameters, performed useful work, and has the same problems with negative numbers.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> template<int First, int Last><br>
> void DoStuff() {<br>
>   if (First + 1 == Last) {<br>
>     DoSomethingHere();  // This gets removed during <0, 0> instantiation in<br>
> the CFG, I think.<br>
>   } else {<br>
>     DoStuff<First, (First + Last)/2>();<br>
>     DoStuff<(First + Last)/2, Last>();<br>
>   }<br>
> }<br>
> DoStuff<0, 1>()<br>
<br>
</div>That's a good example; it's tricky to rewrite in such a way that no<br>
dead code gets instantiated.<br>
You could rewrite it as<br>
<div class="im"><br>
template<int First, int Last><br>
void DoStuff() {<br>
</div>    if (First >= Last) {<br>
        static_assert(First < Last, "Invalid range in DoStuff<First,Last>");<br>
    } else if (First+1 == Last) {<br>
        DoSomethingElse();<br>
    } else {<br>
        const int Middle = (First+Last)/2;<br>
        DoStuff<First, (First==Middle) ? First+1 : Middle>();  // good<br>
old ternary operator<br>
        DoStuff<Middle, Last>();<br>
    }<br>
}<br>
<br>
but I agree that that's ugly.<br>
<div class="im"><br></div></blockquote><div>It's great that you can transform code into something that works, but that doesn't provide anything to the argument.  If Clang came across this code, could it automatically generate the proper code to silence the warning and present it to the user?  What about the general transform for all cases?    I fear that people seeing this warning would prefer to turn off the warning than fix their code.</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
<br>
>  (D) where this diagnostic found bugs in template code?<br>
> struct Value { int num; };<br>
> template<typename T><br>
> struct Wrapper {<br>
>   int num() { return num(); }  // should be V.num;<br>
>   T V;<br>
> };<br>
> Wrapper<Value> Foo;<br>
<br>
</div>Another good example. Given the choice between "warn on both DoStuff<br>
and Wrapper" and "warn on neither DoStuff nor Wrapper", I'd vastly<br>
prefer the former.<br></blockquote><div>Option 3: fix the warning for these cases. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
–Arthur<br>
</font></span></blockquote></div><br></div></div>