On Tue, 2014-04-22 at 09:03 +0100, 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 <[email protected]> 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.
>
> 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)".
Beauty is indeed in the eye of the beholder :)
I prefer my proposal, but I also like yours. It would convert these
fragments (from patch 2):
static void
dump_gimple_switch (pretty_printer *buffer, gimple_switch gs, int spc,
int flags)
[...snip...]
[...later, within pp_gimple_stmt_1:]
case GIMPLE_SWITCH:
dump_gimple_switch (buffer, gs->as_a_gimple_switch (), spc, flags);
break;
to these:
static void
dump_gimple_switch (pretty_printer *buffer, gimple_switch *gs, int spc,
int flags)
[...snip...]
[...later, within pp_gimple_stmt_1:]
case GIMPLE_SWITCH:
dump_gimple_switch (buffer, as_a <gimple_switch> (gs), spc, flags);
break;
Note that this affects the "pointerness" of the types: in the patches I
sent, since is-a.h has:
template <typename T, typename U>
inline T *
^^^ Note how it returns a (T*)
as_a (U *p)
{
gcc_checking_assert (is_a <T> (p));
^^^^^^^^ but uses the specialization of T, not T*
here
return is_a_helper <T>::cast (p);
^^^^^^^^^^^^^^^ and here
}
i.e. in the current proposal, "gimple_switch" is a typedef of a
*pointer* to the GIMPLE_SWITCH subclass:
class gimple_statement_switch
whereas direct use of the is-a.h interface would eliminate the new
typedefs and give us:
class gimple_switch
and all vars and params that took a subclass would convert from status
quo:
gimple some_switch_stmt;
to:
gimple_switch *some_switch_stmt;
I like this API.
One drawback is that it leads to an inconsistency in "pointerness"
between the typedef "gimple", a pointer to the baseclass, and these
subclass types, so you might have local decls looking like this:
gimple some_stmt; /* note how this doesn't have a star... */
gimple_assign *assign_stmt; /* ...whereas these ones do */
gimple_cond *assign_stmt;
gimple_phi *phi;
This could be resolved by renaming
class gimple_statement_base
to
class gimple
and eliminating the "gimple" typedef, so that we might have locals
declared like this:
gimple *some_stmt; /* note how this has gained a star */
gimple_assign *assign_stmt;
gimple_cond *assign_stmt;
gimple_phi *phi;
though clearly that's a hugely invasive patch compared to these ones, or
by renaming:
class gimple_statement_base
to
class gimple_stmt
and *eventually* eliminating the "gimple" typedef, so that we might have
locals declared like this:
gimple_stmt *some_stmt;
gimple_assign *assign_stmt;
gimple_cond *assign_stmt;
gimple_phi *phi;
which gives us a clearer path to the namespace idea that Andrew posted,
if people like that. (basically a search-and-replace of "gimple_" to
"gimple::", or possibly using a "using gimple" decl).
[IIRC we ran into a similar issue with the symtable/callgraph reorg
where we ended up doing a big renamining, but that was just 3 classes]
Alternatively we could change the is-a.h API to eliminate this
discrepancy, and keep the typedefs; giving something like the following:
static void
dump_gimple_switch (pretty_printer *buffer, gimple_switch gs, int spc,
int flags)
[...snip...]
[...later, within pp_gimple_stmt_1:]
case GIMPLE_SWITCH:
dump_gimple_switch (buffer, as_a <gimple_switch> (gs), spc, flags);
break;
which is concise, readable, and avoid the change in pointerness compared
to the "gimple" typedef; the local decls above would look like this:
gimple some_stmt; /* note how this doesn't have a star... */
gimple_assign assign_stmt; /* ...and neither do these */
gimple_cond assign_stmt;
gimple_phi phi;
I think this last proposal is my preferred API, but it requires the
change to is-a.h
Attached is a proposed change to the is-a.h API that elimintates the
discrepancy, allowing the use of typedefs with is-a.h (doesn't yet
compile, but hopefully illustrates the idea). Note how it changes the
API to match C++'s dynamic_cast<> operator i.e. you do
Q* q = dyn_cast<Q*> (p);
not:
Q* q = dyn_cast<Q> (p);
Dave
diff --git a/gcc/is-a.h b/gcc/is-a.h
index bc756b1..a14e344 100644
--- a/gcc/is-a.h
+++ b/gcc/is-a.h
@@ -34,21 +34,21 @@ bool is_a <TYPE> (pointer)
Suppose you have a symtab_node *ptr, AKA symtab_node *ptr. You can test
whether it points to a 'derived' cgraph_node as follows.
- if (is_a <cgraph_node> (ptr))
+ if (is_a <cgraph_node *> (ptr))
....
-TYPE *as_a <TYPE> (pointer)
+TYPE as_a <TYPE> (pointer)
- Converts pointer to a TYPE*.
+ Converts pointer to a TYPE.
You can just assume that it is such a node.
- do_something_with (as_a <cgraph_node> *ptr);
+ do_something_with (as_a <cgraph_node *> *ptr);
-TYPE *dyn_cast <TYPE> (pointer)
+TYPE dyn_cast <TYPE> (pointer)
- Converts pointer to TYPE* if and only if "is_a <TYPE> pointer". Otherwise,
+ Converts pointer to TYPE if and only if "is_a <TYPE> pointer". Otherwise,
returns NULL. This function is essentially a checked down cast.
This functions reduce compile time and increase type safety when treating a
@@ -57,7 +57,7 @@ TYPE *dyn_cast <TYPE> (pointer)
You can test and obtain a pointer to the 'derived' type in one indivisible
operation.
- if (cgraph_node *cptr = dyn_cast <cgraph_node> (ptr))
+ if (cgraph_node *cptr = dyn_cast <cgraph_node *> (ptr))
....
As an example, the code change is from
@@ -70,7 +70,7 @@ TYPE *dyn_cast <TYPE> (pointer)
to
- if (cgraph_node *cnode = dyn_cast <cgraph_node> (node))
+ if (cgraph_node *cnode = dyn_cast <cgraph_node *> (node))
{
....
}
@@ -88,7 +88,7 @@ TYPE *dyn_cast <TYPE> (pointer)
becomes
- varpool_node *vnode = dyn_cast <varpool_node> (node);
+ varpool_node *vnode = dyn_cast <varpool_node *> (node);
if (vnode && vnode->finalized)
varpool_analyze_node (vnode);
@@ -110,7 +110,7 @@ example,
template <>
template <>
inline bool
- is_a_helper <cgraph_node>::test (symtab_node *p)
+ is_a_helper <cgraph_node *>::test (symtab_node *p)
{
return p->type == SYMTAB_FUNCTION;
}
@@ -122,7 +122,7 @@ when needed may result in a crash. For example,
template <>
template <>
inline bool
- is_a_helper <cgraph_node>::cast (symtab_node *p)
+ is_a_helper <cgraph_node *>::cast (symtab_node *p)
{
return &p->x_function;
}
@@ -140,7 +140,7 @@ struct is_a_helper
template <typename U>
static inline bool test (U *p);
template <typename U>
- static inline T *cast (U *p);
+ static inline T cast (U *p);
};
/* Note that we deliberately do not define the 'test' member template. Not
@@ -154,10 +154,10 @@ struct is_a_helper
template <typename T>
template <typename U>
-inline T *
+inline T
is_a_helper <T>::cast (U *p)
{
- return reinterpret_cast <T *> (p);
+ return reinterpret_cast <T> (p);
}
@@ -178,7 +178,7 @@ is_a (U *p)
discussion above for when to use this function. */
template <typename T, typename U>
-inline T *
+inline T
as_a (U *p)
{
gcc_checking_assert (is_a <T> (p));
@@ -189,13 +189,13 @@ as_a (U *p)
the discussion above for when to use this function. */
template <typename T, typename U>
-inline T *
+inline T
dyn_cast (U *p)
{
if (is_a <T> (p))
return is_a_helper <T>::cast (p);
else
- return static_cast <T *> (0);
+ return static_cast <T> (0);
}
#endif /* GCC_IS_A_H */
