This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH 00/89] Compile-time gimple-checking
- From: Andrew MacLeod <amacleod at redhat dot com>
- To: David Malcolm <dmalcolm at redhat dot com>, gcc-patches at gcc dot gnu dot org, rdsandiford at googlemail dot com
- Date: Tue, 22 Apr 2014 09:05:43 -0400
- Subject: Re: [PATCH 00/89] Compile-time gimple-checking
- Authentication-results: sourceware.org; auth=none
- References: <1398099480-49147-1-git-send-email-dmalcolm at redhat dot com> <877g6hhjti dot fsf at talisman dot default>
On 04/22/2014 04:03 AM, Richard Sandiford wrote:
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.
First of all, thanks a lot for doing this. Maybe one day we'll have
the same in rtl :-)
David Malcolm <email@example.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
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
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
"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.
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
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 :-)