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

[David Malcolm <dmalcolm at redhat dot com>]

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

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