This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

On 11/01/2013 06:42 PM, David Malcolm wrote:

!       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?
no, I wasn't implying it did... just mapping out what I think we can go on to do with it.

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

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.
Eventually this is going to be an issue elsewhere as well. Whats the problem with methods in gengtype? it just doesn't parse them? or something deeper and more insidious?
Which results in a similar feel to the new gimple_type, gimple_decl,
etc. classes with my upcoming tree wrappers.  Changing gimple statements
to behave something like this is actually mentioned in the plan, but out
in the future once trees have been taken care of.

I would also plan to create instances for each of the gimple statements
By "instances" presumably you meant "subclasses"?
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.
Its probably pushing it for 4.9... and doesn't really serve much purpose to try do do any of this followup work now. Probably a better task for the next stage 1 anyway when there is time to fully flush it out and prove the various improvements to have full support from everyone :-)
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]

Just expressing my opinion :-)

Presumably we'd want typedefs for the extra subclasses as well?

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?

I would think a typedef for any type that we would directly refer to in the code, like a gimple_omp_clause, gimple_assign, or whatever.

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,

Thanks for looking at this (and for your cleanup work!)

[1] and the patches still need formal review.

Im not the one that has to be sold on this, I'm actually ambivalent to the change this close to the end of stage1... I'm pointing out what we can eventually do with it in case it sways opinion..

I would have considered something similar in a "while" when the other refactoring work reached that point. In some ways it might be better for next stage 1, but perhaps you have outlined enough benefits for it to be attractive to someone today. It seems to be pretty neutral in most other respects.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]