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 <dmalc...@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. > > 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 */