[PATCH] Add new warning to Clang to detect when all code paths in a function has a call back to the function.

[Richard Trieu]

On Wed, Oct 9, 2013 at 4:19 PM, Arthur O'Dwyer <arthur.j.odwyer at gmail.com>wrote:

> On Wed, Oct 9, 2013 at 3:57 PM, Richard Trieu <rtrieu at google.com> wrote:
> > On Wed, Oct 9, 2013 at 2:53 PM, Arthur O'Dwyer <
> arthur.j.odwyer at gmail.com>
> > wrote:
> >>
> >> Well, I do think that you should run this diagnostic over a few real
> >> codebases
> >
> > Already done.  My comments with the patch reflect the results of running
> > this warning over Google code.
> [...]
> >  (B) where this diagnostic gave false positives on non-template code,
> > Nope, didn't find any.
>
> Didn't you find some related to infinite loops? Or else why would you
> put in the special case to disable the warning on
>
>     void foo() { while (true) { ... foo(); ... } }
>
> ?
>
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.

>
> >  (C) where this diagnostic gave false positives on template code, and
> > // Same example as before.
> > template <int value>
> > int sum() {
> >   return value + sum<value/2>;
>
> Yeah, but this example is syntactically incorrect *and* even if the
> syntax errors are fixed it's just a verbose and
> buggy-on-negative-numbers substitute for __builtin_popcount(x-y) *and*
> I've given you the trivial fix — so I discount this example. :)
>
> 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.


> > template<int First, int Last>
> > void DoStuff() {
> >   if (First + 1 == Last) {
> >     DoSomethingHere();  // This gets removed during <0, 0> instantiation
> in
> > the CFG, I think.
> >   } else {
> >     DoStuff<First, (First + Last)/2>();
> >     DoStuff<(First + Last)/2, Last>();
> >   }
> > }
> > DoStuff<0, 1>()
>
> That's a good example; it's tricky to rewrite in such a way that no
> dead code gets instantiated.
> You could rewrite it as
>
> template<int First, int Last>
> void DoStuff() {
>     if (First >= Last) {
>         static_assert(First < Last, "Invalid range in
> DoStuff<First,Last>");
>     } else if (First+1 == Last) {
>         DoSomethingElse();
>     } else {
>         const int Middle = (First+Last)/2;
>         DoStuff<First, (First==Middle) ? First+1 : Middle>();  // good
> old ternary operator
>         DoStuff<Middle, Last>();
>     }
> }
>
> but I agree that that's ugly.
>
> 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.

>
> >  (D) where this diagnostic found bugs in template code?
> > struct Value { int num; };
> > template<typename T>
> > struct Wrapper {
> >   int num() { return num(); }  // should be V.num;
> >   T V;
> > };
> > Wrapper<Value> Foo;
>
> Another good example. Given the choice between "warn on both DoStuff
> and Wrapper" and "warn on neither DoStuff nor Wrapper", I'd vastly
> prefer the former.
>
Option 3: fix the warning for these cases.

>
> –Arthur
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131009/c3b104bb/attachment.html>