This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: "gimple-classes-v2-option-3" git branch committed to svn trunk as r217787


On 11/19/2014 05:24 PM, David Malcolm wrote:
On Wed, 2014-11-19 at 22:36 +0100, Richard Biener wrote:
On November 19, 2014 10:09:56 PM CET, Andrew MacLeod <amacleod@redhat.com> wrote:
On 11/19/2014 03:43 PM, Richard Biener wrote:
On November 19, 2014 8:26:23 PM CET, Andrew MacLeod
<amacleod@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.

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.

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 <> ?

wasnt that my last thought? is_a_call(), as_a_call() and dyn_cast_call () ?

I think lack of <> in identifiers helps us old brains parse faster :-) <> are like ()... many many years of causing a certain kind of break in mental processing. I'm accustomed to single <> these days, but once you get into multiple <>'s I quickly loose track. I find the same thing with ()... hence I'm not a lisp fan :-)

I dont think 'operator *' c++ifies it too much, but I still think operator gimple() would be easier... no extra character at all, and no odd looking dereference of a non-pointer object or double dereference of a pointer. I cant think of how that could get us into trouble... it'll always map to the stmt the iterator currently points to.

Andrew


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]