Re: [PATCH 0/3] Compile-time gimple checking v4

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, May 13, 2014 at 3:27 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Tue, 2014-05-13 at 15:10 +0200, Richard Biener wrote:
>> On Tue, May 13, 2014 at 2:37 PM, Michael Matz <matz@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.

Well, we can then also simply rename the functions first.  And not bother
touching each line twice eventually.

> 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)).

Eh :/  But then we can as well keep the function as gswitch_label
(switch_stmt, i).

C++ isn't really a good greppable language.  But well.  We voted for it.

Richard.

> (and perhaps omit the prefix when it's unambiguous e.g.
> gimple_cond_make_false would become
> simply   cond_stmt->make_false ()  ?)

>
> Dave
>
>