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

[Andrew MacLeod <amacleod@redhat.com>]

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
> ENABLE_CHECKING builds).
>
> 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"?
yeah.
>> 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,
> though.
>
> Thanks for looking at this (and for your cleanup work!)
>
> Dave
> [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.


Andrew