On Tue, Apr 22, 2014 at 09:05:43AM -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.That seems kind of unlikely to me given that other things depend on the enum having an element for each "sub class", but who knows. > 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 fwiw I don't really have an opinion either way. > 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. I'm not convinced having to change it would be *that* bad, I would expect most of the change could be done with sed. > If this is purely for verbosity, I think we can find better ways to reduce > it... Is there any other reason? > > > > >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. as well as the typedef gimple_bind being what's used all over, so it would seem to make sense to rename the class and drop the typedef. > > > >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. That seems nice, but I'd worry about name conflicts with rtl or tree types or something. > There will be places in the code where we have used something like > gimple switch_stmt = blah(); We already have things like loop loop = get_loop (); so conflicts like this don't seem *that* terrible, and I guess we don't absolutely need to rename stuff. > 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. Well, it would be nice if the typename wasn't repeated twice, but I fear there's no way out of that without auto. > I see the gimple part of the name as being redundant. If we're really I'd tend to agree accept... > concerned about it, put the whole thing inside a namespace, say 'Gimple' We'd need to beet gengtype into dealing with that, I'm not sure exactly how hard that is. > 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. > > 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 :-) if the rest of them are getting renamed then I think renaming gimple too would make sense, and that's what sed is for. Trev > Andrew > > >
signature.asc
Description: Digital signature
