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 Wed, 2014-11-19 at 17:24 -0500, 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 <> ?

Answering my own question, user-defined conversion operators, though
should they as_a or dyn_cast?

Here's an example where they mean "as_a", radically shortening the
conversion (shorter, in fact, that the pre-merger code):

diff --git a/gcc/asan.c b/gcc/asan.c
index be28ede..890e58b 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 = *iter;
 
   gcc_checking_assert (gimple_call_builtin_p (call, BUILT_IN_NORMAL));
 
diff --git a/gcc/gimple-iterator.h b/gcc/gimple-iterator.h
index fb6cc07..52106fa 100644
--- a/gcc/gimple-iterator.h
+++ b/gcc/gimple-iterator.h
@@ -33,6 +33,8 @@ struct gimple_stmt_iterator
      block/sequence is removed.  */
   gimple_seq *seq;
   basic_block bb;
+
+  operator gcall * ();
 };
 
 /* Iterator over GIMPLE_PHI statements.  */
@@ -331,4 +333,11 @@ gsi_seq (gimple_stmt_iterator i)
   return *i.seq;
 }
 
+inline
+gimple_stmt_iterator::operator gcall * ()
+{
+  return as_a <gcall *> (gsi_stmt (*this));
+}
+
+
 #endif /* GCC_GIMPLE_ITERATOR_H */


(again, I only checked that it compiles)

Dave


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