Re: [PATCH 0/6] Conversion of gimple types to C++ inheritance (v3)

[Trevor Saunders <tsaunders@mozilla.com>]

> > It is a start, but it doesnt do the rest of the work that needs doing to 
> > really take advantage of it... which will be extensive.
> > for instance, we should change:
> > 
> >    static inline void
> > ! gimple_call_set_lhs (gimple gs, tree lhs)
> >    {
> > -   GIMPLE_CHECK (gs, GIMPLE_CALL);
> >      gimple_set_op (gs, 0, lhs);
> > to
> >      static inline void
> > ! gimple_call_set_lhs (gimple_statement_call *gs, tree lhs)
> >    {
> >      gimple_set_op (gs, 0, lhs);
> > 
> > 
> > but then every location that calls it needs an appropriate change:
> > 
> > !       gimple call;
> > !       call = gimple_build_call_vec (build_fold_addr_expr_loc (0, 
> > alias), vargs);
> >          gimple_call_set_lhs (call, atree);
> > 
> > --- 1518,1524 ----
> > 
> > !       gimple_statement_call *call;
> > !       call = as_a<gimple_statement_call> (gimple_build_call_vec 
> > (build_fold_addr_expr_loc (0, alias), vargs));
> >          gimple_call_set_lhs (call, atree);
> > 
> > And in fact there is a ripple effect to then change 
> > gimple_build_call_vec to simply return a gimple_statement_call *... Then 
> > this doesn't look as ugly either...
> > 
> > !       gimple_statement_call *call;
> > !       call = gimple_build_call_vec (build_fold_addr_expr_loc (0, 
> > alias), vargs);
> >          gimple_call_set_lhs (call, atree);
> > 
> > that is looking much better :-)
> 
> I'd love to make use of compile-time type-safety like this, but does
> this have to gate this initial step?

 I don't think Andrew was suggesting it should, but fwiw I agree with
 you one step at a time.

> The transition to strongly-typed getters/setters could be done one
> at-a-time, I guess.

I actually have a slightly different suggestion, see below.

> > Leaving the names as they are should be ok, but the I'd also add a 
> > typedef for the pointer without the 'statement' in the name.. ie
> >      typedef gimple_statement_call *gimple_call;
> > That seems in line with what we do with 'gimple' right now and the short 
> > form is the type we'd normally use.
> > 
> > That adds a touch of difficulty with "as_a", since that requires the 
> > type name, not the shorthand pointer.... so you have something like
> > gimple_call call = as_a <gimple_statement_call> blah().
> > I think as the changes to use the gimple_call type are pushed through 
> > all the callers and callee's, the requirement of as_a and the long name 
> > being ugly begins to rapidly disappear...  it'll only exist in the core 
> > creation routines and no one will usually see it.   So I don't *think* 
> > this is an issue...  but it is an ugly transition if its only partially 
> > done.
> 
> (nods)
> 
> > And eventually we can pull the accessor routines and others into the 
> > class itself:
> >         gimple_call call;
> >         call = gimple_build_call_vec (build_fold_addr_expr_loc (0, 
> > alias), vargs);
> >         call->set_lhs (atree);
> 
> Nice.  It's readable (IMHO), and the type-checking (that it's a call) is
> enforced at compile-time, rather than at run-time (and only in
> ENABLE_CHECKING builds).

Well, if you never need to downcast its always enforced by the type
system.

> Sadly, this may need some further gengtype work: the simple inheritance
> support I'm using in this patch series will barf if it sees methods
> within a class.  So I can have a look at supporting methods in gengtype,
> I guess.   Though the change above of gimple_call_set_lhs to accepting a
> subclass ptr gives a middle-ground.

hm, I have a patch around that adds member functions to symtab_node and
gentype doesn't fall over on it, is it just sub classes maybe?

 Anyway to avoid churn multiple times / ugly intermediates what I think
 we should do is:

 1. add methods to the sub classes as appropriate (fixing gentype first
 if needed).
 2. change creator functions to return dirived types.
 3. when we already have the sub type replace the function with the
 method.
 4. when all uses of the function are gone remove it.

> > that don't have them now, like gimple_statement_assign. Its lumped in as 
> > a general GSS_WITH_MEM_OPS, so there is no gimple_statement_assign 
> > class/struct to use.
> 
> > It would really be nice to use the DEFGSCODE macro and gimple.def to 
> > make this happen automagically somehow... Its tantalizingly close now I 
> > think, especially combined with the info in gsstruct.def... Although if 
> > we are moving to proper classes eventually its probably better to 
> > explicitly write the required class for a statement kind.
> 
> I think writing them out explicitly is better than doing them with
> preprocessor magic: if we go down the route described above of wanting
> to add methods, the various subclasses will eventually gain their own
> methods, so we'll want code rather than building it all from .def files.

agreed.

> > That all said, this change enables that work to proceed if someone wants 
> > to do it.
> > 
> > My question is: Is anyone going to do it, and if so,  who and when? :-)
> 
> I'd be up for working on a followup patch that adds such subclasses, and
> I'd be up for changing the accessors to give up compile-time
> type-safety.  I'd do it one accessor at a time.

I'd be happy to work on this too.

> > > Again, as noted in the earlier patch series, the names of the structs
> > > are rather verbose.  I would prefer to also rename them all to eliminate
> > > the "_statement" component:
> > >    "gimple_statement_base" -> "gimple_base"
> > >    "gimple_statement_phi"  -> "gimple_phi"
> > >    "gimple_statement_omp"  -> "gimple_omp"
> > > etc, but I didn't do this to mimimize the patch size.  But if the core
> > > maintainers are up for that, I can redo the patch series with that
> > > change also, or do that as a followup.
> > 
> > As mentioned, I'd rather see the short names be typedefs for pointers to 
> > the long names since we're typically using the pointers.
> 
> [FWIW, I'm not a fan of such typedefs, but I'll defer to you in this]

agreed, especially since C++ lets us omit the struct / class part
anyway.

Trev

> Also, I take it we'd use the "printable_name" from gimple.def - though
> would we use it for the subclass name, or for the ptr typedef?
> 
> > Other than stomping on the same bits at the moment (less so once 
> > gimple-stmt.[ch] is split out), these changes are completely orthogonal, 
> > but in line with with my direction.  I think It should be done sooner or 
> > later....  My preference is to also see the follow up work carried out 
> > as well.  Or at least a plan for it.
> 
> So you like the approach, provided I commit to the followup work? [1]
> 
> OK, I'll try to create some followup patches next week.  I'd prefer to
> get the conversion to inheritance into trunk sooner rather than later,
> though.
> 
> Thanks for looking at this (and for your cleanup work!)
> 
> Dave
> [1] and the patches still need formal review.
>