Re: [PATCH 1/2] add staticly checked label_nuses accessors

[Trevor Saunders <tsaunders at mozilla dot com>]

On Fri, Sep 05, 2014 at 05:57:13PM -0400, David Malcolm wrote:
> On Fri, 2014-09-05 at 10:43 -0600, Jeff Law wrote:
> > On 09/05/14 09:32, tsaunders@mozilla.com wrote:
> > > From: Trevor Saunders <tsaunders@mozilla.com>
> > >
> > > Hi,
> > >
> > >   Doing this means we get to remove a fair chunk of runtime checking.
> > >
> > > bootstrapped + regtested on x86_64-unknown-linux-gnu with no regressions.
> > > config-list.mk with this and the next patch is ongoing. ok?
> > >
> > > Trev
> > >
> > > gcc/
> > >
> > > 	* config/i386/i386.c, config/i386/i386.md, config/mep/mep.c,
> > > 	config/mips/mips.c, config/nios2/nios2.c, config/s390/s390.c,
> > > 	config/sh/sh.c, reorg.c: Use rtx_code_label::nuses and
> > > 	rtx_code_label::set_nuses where possible.
> > > 	* rtl.h (rtx_code_label::nuses): New member function.
> > > 	(rtx_code_label::set_nuses): Likewise.
> > Is there some reason why we don't fix every reference to LABEL_NUSES to 
> > use the new member functions?  My concern here is that we've now got two 
> > ways actively used to get at that information, the old LABEL_NUSES and 
> > the new member functions.  Obviously we really just want one blessed way 
> > to get/set that information.  If this is a short term situation, then 
> > I'd be inclined to say OK, but I don't see further patches which sort 
> > out the ~200 or so LABEL_NUSES users.
> > 
> > THe approach David has taken for similar situations has been to first 
> > introduce the get/set methods, convert everything to set the get/set 
> > method, dealing with the type fallout that occurrs around that, then 
> > collapsing back to the old macro.  Is there some reason we can not or 
> > should not do that here?
> 
> If it's a macro that's simply a direct field access into a struct (e.g.
> BB_HEADER), then yes, but for those that are wrappers around e.g. XEXP
> I've been keeping them as inline functions, so that the type system
> detects incorrect node types as compile-time errors.
> 
> One other aspect of my approach is that (believe it or not) I'm trying
> to minimize the size of the changes, to avoid introducing pain when
> backporting bugfixes from trunk to the branches.
> 
> My goal here is type-safety, with readability as a secondary benefit.  I
> think it's a good idea for us to add methods that let us replace e.g.
> XEXP (x, 0) accessors with descriptive names, and have been doing so,
> and I prefer doing this as methods for new code.  However, when the
> accessor already has a descriptive name, like LABEL_NUSES, I think it's
> enough to convert them to inline functions and tighten up the params and
> return type to express things.  I'm not sure the cost/benefit of
> *additionally* converting them to be methods is worth it, given that it
> means changing the spelling at every callsite.

So, in this case it seems to me you basically have two options

A. change the macro to an inline function, and fix up all the callers to
pass the right type.  Then rebase that into some sort of reasonable
patch series.

b. add a function with a different name and convert users piece meal,
and then remove the old macro (I guess you can then rename the new
function to the old macro if you like).

of those I prefer b because it means you can convert call sites one at a
time and its easy to know which have been delt with.

I also do think the advantages of using members outways the cost.

For one thing functions with all caps names are just weird.  I think the
more important reason though is that it will help make rtx_insn be a
separate class sometime in the far future, since at some point we can
make its inheritance from rtx_def protected to chase down things that
try to convert from rtx_insn to rtx.  There's also the argument that its
inconsistant to have some things be members and others be functions.

Trev

> 
> Dave
>