Re: RFC: Add C++ warnings for unnecessarily general return types

[Jeff Law <law at redhat dot com>]

On 05/29/2015 11:39 AM, Richard Sandiford wrote:
> One of the main early aims of the rtl refactoring is to finally separate
> instructions (rtx_insns) from other rtxes.  I thought it would help if the
> compiler could warn about cases where a function returns rtx when it
> could return rtx_insn*, or where a variable has type rtx but could have
> type rtx_insn*.
>
> This patch tries to implement something along those lines.  There are two
> warning options, one for function returns and one for variables.  There are
> obviously many good reasons why a functon or variable might have a more general
> type than it might appear to need, so the options certainly aren't -Wall or
> -Wextra material.  But it might be worth having them as stage 2 and 3 warnings.
I can't speak to the implementation as it hits the C++ front-end where 
my knowledge approaches zero.  But I can say that I really like what 
you're trying to do here.  I can easily see how it'd be useful for GCC.

The big question in my mind is how useful would this be on other 
codebases --  I have a hard time believing that GCC is the only codebase 
which has these problems.

> I have a local patch that fixes all instances of the warnings in an
> x86_64-linux-gnu bootstrap.
Very cool.  It allows us to find a whole class of strengthening cases 
that we can get virtually for free.  Leaving more time for the harder stuff.


> This is very much an RFC.  Maybe the options are too special-purpose
> to be worth including.  Or, if they are worth including, there are
> probably interesting corner cases that I've not thought about.
>
> The option did find a couple of useful things besides rtx->rtx_insn*
> though.  For example, in ipa-comdats.c:enqueue_references we have:
>
> 	    symtab_node *node = edge->callee->ultimate_alias_target ();
>
> 	    /* Always keep thunks in same sections as target function.  */
> 	    if (is_a <cgraph_node *>(node))
> 	      node = dyn_cast <cgraph_node *> (node)->function_symbol ();
> 	    if (!node->aux && node->definition)
> 	      {
> 		 node->aux = *first;
> 		 *first = node;
> 	      }
>
> ultimate_alias_target always returns a cgraph_node*, so the is_a is
> always true.  There are also two cases where we have unnecessary
> safe_as_a <rtx_insn *>s.
And as I've stated before, the is_a, as_a and safe_as_a are in my mind 
are really just markers for the boundaries of where we've pushed the 
rtx_insn * changes.  I hope and expect that as the work as a whole 
progresses many of those will just disappear because they're not needed.

Thanks for working on this.  I hope the C++ maintainers look upon it 
favorably.

jeff