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] Add gimple-compat.h (was Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign)

On 11/14/14 08:27, David Malcolm wrote:

I just don't like all the as_a/is_a stuff enforced everywhere,
it means more typing, more temporaries, more indentation.
So, as I view it, instead of the checks being done cheaply (yes, I think
the gimple checking as we have right now is very cheap) under the
hood by the accessors (gimple_assign_{lhs,rhs1} etc.), those changes
put the burden on the developers, who has to check that manually through
the as_a/is_a stuff everywhere, more typing and uglier syntax.
I just don't see that as a step forward, instead a huge step backwards.
But perhaps I'm alone with this.
Can you e.g. compare the size of - lines in your patchset combined, and
size of + lines in your patchset?  As in, if your changes lead to less
typing or more.
So, I'm chiming in a bit late, but just want to touch on a few things.

First, as I've stated before, I see as_a/is_a as generally a wart for things we still need to cleanup and redesign. I do not want to see them sprinkled throughout GCC. If we find ourselves adding a bunch of these, then we've got some redesign/rethinking that needs to be done.

Yes, I know some will be necessary and some are more like markers for the limits of where we are with the gimple class work, particularly since we're trying to stage in this work rather than convert everything at once (which I believe, realistically, is impossible).

I see two ways out here.  One is to add overloads to all the functions
taking the special types like
IIRC Andrew was doing similar things as a temporary measure in the gimple/tree type work as well. Basically the overloads were to allow the two schemes to co-exist while conversions were done with the express intent that the overloads were to disappear when conversion is complete. I'd be comfortable with a similar mechanism for this work as well.

Option 3: only convert the "easy" accessors: the ones I already did in
the /89 patch kit, as reviewed by Jeff, and rebased by me recently,
which is this 92-patch kit:
   "[gimple-classes, committed 00/92] Initial slew of commits":
Doing so converts about half of the gimple_foo_ accessors to taking a
gfoo *, giving a mixture of GIMPLE_CHECK vs subclass use.   I believe
the quality of those patches was higher than the later ones on the
branch: I was doing the places that didn't require the invasive/verbose
changes seen in the later patches.  Shelve the remaining ~80
increasingly ugly patches, starting a new branch to contain just the
good ones.
And this would be my preferred option for where we are today. That ~89 kit was a step in the right direction. I think going beyond that for the close of stage1 was ambitious :-)

Works for me as well.  The compat solution looks somewhat appealing
as we can then incrementally fix up things rather than requiring to
mass-convert everything.
Exactly. And it's real easy to see what's depending on those overloads as one can simply remove them and try to build. In many ways, I'd prefer the temporary overload solution for the next round of work in this space so that conversions can occur piecemeal instead of in large series patchsets.

I've got no objection if we have the compat hack in now. I haven't reviewed that work, just no philosophical objections :-) I would be opposed to pushing the gimple class work further than that prior to the next stage1 opening.


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