"gimple-classes-v2-option-3" git branch committed to svn trunk as r217787
diff mbox

Message ID CAFiYyc2pXh+sChUP91Yn3e9EF87zR7KURh6EiF7YdcThZ-2gBA@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener Nov. 20, 2014, 1:12 p.m. UTC
On Wed, Nov 19, 2014 at 11:24 PM, David Malcolm <dmalcolm@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 <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.

Looks good.  Note that


   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.

Comments

Michael Matz Nov. 20, 2014, 3:34 p.m. UTC | #1
Hi,

On Thu, 20 Nov 2014, Richard Biener wrote:

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

And?  Nobody says we can't have nice looking code even with C++.

> Now please make it AT LEAST CONSISTENT.

True.

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

Oh god, yes.  Please!  IMHO they don't accomplish much, but make code 
harder to visually parse.  They don't accomplish much because you 
have to write the snippets that check validity of conversions anyway, so 
they can just as well be written as proper methods or global functions, or 
even just conversion operators.  Nothing forces us to implement these 
snippets as noisy template specializations like:

  template <>
  template <>
  inline bool
  is_a_helper <cgraph_node *>::test (symtab_node *p)
  {
    return p->type == SYMTAB_FUNCTION;
  }

instead of the more mundane means.  And once you have those snippets as 
normal functions, you can just as well call them like they are functions, 
making the using side of those conversion also look nicer.


Ciao,
Michael.
Richard Biener Nov. 20, 2014, 4:13 p.m. UTC | #2
On Thu, Nov 20, 2014 at 4:34 PM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Thu, 20 Nov 2014, Richard Biener wrote:
>
>> > 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.
>
> And?  Nobody says we can't have nice looking code even with C++.
>
>> Now please make it AT LEAST CONSISTENT.
>
> True.
>
>> > 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.
>
> Oh god, yes.  Please!  IMHO they don't accomplish much, but make code
> harder to visually parse.  They don't accomplish much because you
> have to write the snippets that check validity of conversions anyway, so
> they can just as well be written as proper methods or global functions, or
> even just conversion operators.  Nothing forces us to implement these
> snippets as noisy template specializations like:
>
>   template <>
>   template <>
>   inline bool
>   is_a_helper <cgraph_node *>::test (symtab_node *p)
>   {
>     return p->type == SYMTAB_FUNCTION;
>   }
>
> instead of the more mundane means.  And once you have those snippets as
> normal functions, you can just as well call them like they are functions,
> making the using side of those conversion also look nicer.

True.  I don't remember exactly but exclusively using member functions
wasn't in the list of proposals that ended up with us doing as_a/is_a
as it is done now.

Can unions / PODs have such member functions?  Just thinking about
a reason why it wasn't proposed.

Btw, I don't see as_a/is_a/dyn_cast as super-ugly - it's actually a
perfectly fine C++-way of doing RTTI.

I also guess that it requires less code in the actual implementation
as we share one helper for all three operations.  Of course we
could macroize the member function implementation in some
clever way ....

That said - we have a substantial amount of code using as_a/is_a/dyn_cast
and I don't think it's appropriate at this point to change all of it
to a different
mechanism.  Proposals with example patches are of course welcome, but
beware that if you want to succeed here then as-a.h needs to go ;)

Thanks,
Richard.

>
> Ciao,
> Michael.

Patch
diff mbox

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