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 00/89] Compile-time gimple-checking

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


David Malcolm <> 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);

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

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


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


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