Re: [PATCH 00/89] Compile-time gimple-checking

[Andrew MacLeod <amacleod at redhat dot com>]

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 <dmalcolm@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?

> 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.

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 :-)

Andrew