On Tue, 2014-05-13 at 15:10 +0200, Richard Biener wrote:
> On Tue, May 13, 2014 at 2:37 PM, Michael Matz <m...@suse.de> wrote:
> > Hi,
> >
> > On Mon, 12 May 2014, David Malcolm wrote:
> >
> >> The "gfoo" type names are pleasantly terse, though I'm a little unhappy
> >> about how they no longer match the prefix of the accessor functions e.g.
> >> gimple_switch_num_labels (const gswitch *gs)
> >> vs
> >> gimple_switch_num_labels (const gimple_switch *gs)
> >> But it works.
> >
> > That could also be changed with a followup to make it consistent again
> > (i.e. rename the accessors to gswitch_num_labels). I'd be in favor of
> > such renaming later.
>
> Yeah, or go all the way to member functions.
>
> I'd like to see this addresses as followup, together with a discussion
> on whether we want standalone or member functions here.
Or both, perhaps as a transition strategy e.g. something like:
struct GTY((tag("GSS_WITH_OPS")))
gswitch : public gimple_with_ops
{
public:
unsigned num_labels () const
{
unsigned num_ops = num_ops ();
gcc_gimple_checking_assert (num_ops > 1);
return num_ops - 1;
}
};
static inline unsigned
gimple_switch_num_labels (const gswitch *gs)
{
return gs->num_labels ();
}
where the accessor function simply calls the member fn??? This
both-worlds approach might not be desirable as a end-goal, but might be
a useful tactic when creating the patches.
FWIW, if we want to do a grand renaming - which would touch most lines
in the middle-end - I'd prefer to go the whole way and have member
functions (though the gimple_ -> gswitch_ would be much easier to
automate :) )
I can have a look at making the data fields be private too, I guess.
(with judicious use of "friend", it may be possible to do that patch
without needing to make the accessors be members).
An idea I had for a method-naming convention: keep the type as a prefix
of the name, so that instead of:
(A) tree label = gimple_switch_label (switch_stmt, i);
becoming:
(B) tree label = switch_stmt->label (i);
you'd have:
(C) tree label = switch_stmt->switch_label (i);
^^^^^^^ note the prefix
which might allow us to keep most of the "grepability" of the existing
code (you can't easily find "label" *method* calls in approach (B),
whereas "switch_label" should give much better results in approach (C)).
(and perhaps omit the prefix when it's unambiguous e.g.
gimple_cond_make_false would become
simply cond_stmt->make_false () ?)
Dave
