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.
