On Wed, Nov 19, 2014 at 11:24 PM, David Malcolm <dmalc...@redhat.com> wrote: > On Wed, 2014-11-19 at 22:36 +0100, Richard Biener wrote: >> On November 19, 2014 10:09:56 PM CET, Andrew MacLeod <amacl...@redhat.com> >> wrote: >> >On 11/19/2014 03:43 PM, Richard Biener wrote: >> >> On November 19, 2014 8:26:23 PM CET, Andrew MacLeod >> ><amacl...@redhat.com> wrote: >> >>> On 11/19/2014 01:12 PM, David Malcolm wrote: >> >>> >> >>>> (A) could become: >> >>>> >> >>>> greturn *stmt = gsi->as_a_greturn (); >> >>>> >> >>>> (B) could become: >> >>>> >> >>>> stmt = gsi->dyn_cast <gcall *> (); >> >>>> if (!stmt) >> >>>> or: >> >>>> >> >>>> stmt = gsi->dyn_cast_gcall (); >> >>>> if (!stmt) >> >>>> >> >>>> or maybe: >> >>>> >> >>>> stmt = gsi->is_a_gcall (); >> >>>> if (!stmt) >> >>>> >> >>>> An earlier version of my patches had casting methods within the >> >>>> gimple_statement_base class, which were rejected; the above >> >proposals >> >>>> would instead put them within gimple_stmt_iterator. >> >>>> >> >>> I would like all gsi routines to be consistent, not a mix of >> >functions >> >>> and methods. >> >>> so something like >> >>> >> >>> stmt = gsi_as_call (gsi); >> >>> stmt = gsi_dyn_call (gsi); >> >>> >> >>> or we need to change gsi_stmt() and friends into methods and access >> >>> them >> >>> as gsi->stmt ().. which is possibly better, but that much more >> >>> intrusive again (another 2000+ locations). >> >>> If we switched to methods everywhere for gsi, I'd prefer something >> >like >> >>> gsi->as_a_call () >> >>> gsi->is_a_call () >> >>> gsi->dyn_cast_call () >> >>> >> >>> I think its more readable... and it removes a dependency on the >> >>> implementation.. so if we ever decide to change the name of 'gcall' >> >>> down >> >>> the road to using a namespace, and make it gimple::call or whatever, >> >we >> >>> >> >>> wont have to change every single gsi-> location which has a >> >templated >> >>> use of the type. >> >>> >> >>> I'm also think this sort of thing could probably wait until next >> >stage >> >>> 1.. >> >>> >> >>> my 2 cents... >> >> Why not as_a <gassign *> (*gsi)? It would >> >> Add operator* to gsi of course. >> >> >> >> Richard. >> >> >> >> >> > >> >I could live with that form too. >> > >> >we often have an instance of gimple_stmt_iterator rather than a pointer >> > >> >to it, so wouldn't "operator gimple *()" to implicitly call gsi_stmt() >> > >> >when needed work better? (or "operator gimple ()" before the next >> >change) .. >> >> Not sure. The * matches how iterators work in STL... >> >> Note that for the cases where we pass a pointer to an iterator we can change >> those to use references to avoid writing **gsi. >> >> Richard. >> >> >Andrew > > I had a go at adding an operator * to gimple_stmt_iterator, using it > everywhere that we do an as_a<> or dyn_cast<> on the result of a > gsi_stmt, to abbreviate the gsi_stmt call down to one character. > > Patch attached; only lightly smoketested; am posting it for the sake of > discussion.
Looks good. Note that diff --git a/gcc/asan.c b/gcc/asan.c index be28ede..d06d60c 100644 --- a/gcc/asan.c +++ b/gcc/asan.c @@ -1902,7 +1902,7 @@ instrument_builtin_call (gimple_stmt_iterator *iter) return false; bool iter_advanced_p = false; - gcall *call = as_a <gcall *> (gsi_stmt (*iter)); + gcall *call = as_a <gcall *> (**iter); should be "fixed" by making instrument_builtin_call take a reference to the iterator so the above becomes gcall *call = as_a <gcall *> (*iter); probably not possible in 100% of all cases (where we sometimes pass NULL as the iterator pointer) but in most. > I don't think this API will make the non-C++-fans happier; I think the > objection to the work I just merged is that it's adding more C++ than > those people are comfortable with. How so? It's already super-ugly in those views. We decided to get C++. Now we have it. Now please make it AT LEAST CONSISTENT. > So although the attached patch makes things shorter (good), it's taking > things in a "more C++" direction (questionable). I'd hoped to paper > over the C++ somewhat. > > I suspect that any API which requires the of < > characters within the > implementation of a gimple pass to mean a template is going to give > those less happy with C++ a visceral "ugh" reaction. I wonder if > there's a way to spell these things that's concise and which doesn't > involve <> ? Only if you drop as_a/is_a/dyn_cast everywhere. Richard.