On Tue, 2014-04-22 at 09:05 -0400, Andrew MacLeod wrote:
> On 04/22/2014 04:03 AM, Richard Sandiford wrote:
> > First of all, thanks a lot for doing this. Maybe one day we'll have
> > the same in rtl :-)
> >
> > But...
> >
> > David Malcolm <dmalc...@redhat.com> writes:
> >> In doing the checked downcasts I ran into the verbosity of the as_a <>
> >> API (in our "is-a.h"). I first tried simplifying them with custom
> >> functions e.g.:
> >>
> >> static inline gimple_bind as_a_gimple_bind (gimple gs)
> >> {
> >> return as_a <gimple_statement_bind> (gs);
> >> }
> >>
> >> but the approach I've gone with makes these checked casts be *methods* of
> >> the gimple_statement_base class, so that e.g. in a switch statement you
> >> can write:
> >>
> >> case GIMPLE_SWITCH:
> >> dump_gimple_switch (buffer, gs->as_a_gimple_switch (), spc, flags);
> >> break;
> >>
> >> where the ->as_a_gimple_switch is a no-op cast from "gimple" to the more
> >> concrete "gimple_switch" in a release build, with runtime checking for
> >> code == GIMPLE_SWITCH added in a checked build (it uses as_a <>
> >> internally).
> >>
> >> This is much less verbose than trying to do it with as_a <> directly, and
> >> I think doing it as a method reads better aloud (to my English-speaking
> >> mind, at-least):
> >> "gs as a gimple switch",
> >> as opposed to:
> >> "as a gimple switch... gs",
> >> which I find clunky.
> >>
> >> It makes the base class a little cluttered, but IMHO it hits a sweet-spot
> >> of readability and type-safety with relatively little verbosity (only 8
> >> more characters than doing it with a raw C-style cast). Another
> >> advantage of having the checked cast as a *method* is that it implicitly
> >> documents the requirement that the input must be non-NULL.
> > ...FWIW I really don't like these cast members. The counterarguments are:
> >
> > - as_a <...> (...) and dyn_cast <...> (...) follow the C++ syntax
> > for other casts.
> >
> > - the type you get is obvious, rather than being a contraction of
> > the type name.
> >
> > - having them as methods means that the base class needs to aware of
> > all subclasses. I realise that's probably inherently true of gimple
> > due to the enum, but it seems like bad design. You could potentially
> > have different subclasses for the same enum, selected by a secondary
> > field.
> I'm not particularly fond of this aspect as well... I fear that someday
> down the road we would regret this decision, and end up changing it all
> back to is_a<> and friends.... These kind of sweeping changes we
> ought to try very hard to make sure we only have to do it once.
>
> If this is purely for verbosity, I think we can find better ways to
> reduce it... Is there any other reason?
There was also the idea that a method carries with it the implication
that the ptr is non-NULL... but the main reason was verbosity.
I think that with a change to is-a.h to better support typedefs, we can
achieve a relatively terse API; see:
http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01334.html
> > Maybe I've just been reading C code too long, but "as a gimple switch...gs"
> > doesn't seem any less natural than "is constant...address".
> >
> > Another way of reducing the verbosity of as_a would be to shorten the
> > type names. E.g. "gimple_statement" contracts to "gimple_stmt" in some
> > places, so "gimple_statement_bind" could become "gimple_stmt_bind" or
> > just "gimple_bind". "gimple_bind" is probably better since it matches
> > the names of the accessors.
> >
> > If the thing after "as_a_" matches the type name, the "X->as_a_foo ()"
> > takes the same number of characters as "as_a <foo> (X)".
> >
>
> I was running into similar issues with the gimple re-arch work...
>
> One thing I was going to bring up at some point was the possibility of
> some renaming of types. In the context of these gimple statements, I
> would propose that we drop the "gimple_" prefix completely, and end up
> with maybe something more concise like
>
> bind_stmt,
> switch_stmt,
> assign_stmt,
> etc.
>
> There will be places in the code where we have used something like
> gimple switch_stmt = blah();
> so those variables would have to be renamed... and probably other
> dribble effects... but it would make the code look cleaner. and then
> as_a<>, is_a<> and dyn_cast<> wouldn't look so bad.
>
> I see the gimple part of the name as being redundant. If we're really
> concerned about it, put the whole thing inside a namespace, say
> 'Gimple' and then all the gimple source files can simply start with
> "using namespace Gimple;" and then use 'bind_stmt' throughout. Non
> gimple source files could then refer to it directly as
> Gimple::bind_stmt... This would tie in well with what I'm planning
> to propose for gimple types and values.
That would require teaching gengtype about namespaces (rather than the
current hack), which I'd prefer to avoid.
> Of course, it would be ideal if we could use 'gimple' as the namespace,
> but that is currently taken by the gimple statement type... I'd even go
> so far as to propose that 'gimple' should be renamed 'gimple::stmt'..
> but that is much more work :-)
I'm not at all keen on that further suggestion: I wanted to make this
patch series as minimal as possible whilst giving us the compile-time
tracking of gimple codes. Although people's inboxes may find this
surprising, I was trying to be conservative with the patch series :) [1]
We're both working on large changes that improve the type-safety of the
middle-end: this patch series affects statements, whereas AIUI you have
a branch working on expressions and types. How do we best co-ordinate
this so that we don't bitrot each other's work, so that the result is
reviewable, and the changes are understandable in, say, 5 years time?
My plan was to do the statement work as a (large) series of small
patches against trunk, trying to minimize the number of lines I touch,
mostly function and variable decls with a few lines adding is_a and
dyn_cast, whereas your change AIUI by necessity involves more
substantial changes to function bodies. I think we can only have zero
or one such "touch every line" change(s) landing at once.
Dave
[1] for example, a more aggressive cleanup would be to convert the
accessors to be methods of the underlying statement classes, and make
all the data fields be private. But the cost/benefit tradeoff doesn't
appeal: doing so would touch basically every line of code in the middle
end, and it isn't necessary to give us the compile-time checking
benefits that motivates this patch series [potentially we could use
"friend" decls to allow the data fields to be private whilst granting
access to the existing accessors, I guess].
