Re: gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
On Thu, Nov 20, 2014 at 12:05 AM, Andrew MacLeod amacl...@redhat.com wrote: 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 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. 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 think we want to have a consistent style across GCC even if seen as ugly to some people. Thus having (member) functions for conversion in some cases and as_a templates in others is bad. C++ was supposed to make grok GCC easier for newbies - this is exactly making it harder (not that I believe in this story at all...) 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. I dislike conversion operators. Why is 'operator *' bad? It's exactly how iterators are supposed to work - after all the gsi stuff was modeled after STL iterators! So that's a definitive no from me to is_a_call () as_a_call () etc. Richard. Andrew
Re: gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
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.
Re: gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
On 11/20/2014 08:08 AM, Richard Biener wrote: On Thu, Nov 20, 2014 at 12:05 AM, Andrew MacLeod amacl...@redhat.com wrote: 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 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. 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 think we want to have a consistent style across GCC even if seen as ugly to some people. Thus having (member) functions for conversion in some cases and as_a templates in others is bad. C++ was supposed to make grok GCC easier for newbies - this is exactly making it harder (not that I believe in this story at all...) 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. I dislike conversion operators. Why is 'operator *' bad? It's exactly how iterators are supposed to work - after all the gsi stuff was modeled after STL iterators! So that's a definitive no from me to is_a_call () as_a_call () etc. Richard. Fine by me, Just running through the options to make sure we know what we are getting :-) Andrew
Re: gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
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.
Re: gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
On Thu, Nov 20, 2014 at 4:34 PM, Michael Matz m...@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.
gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
I've committed the cut-down version of the gimple statement subclasses work to svn trunk [specifically the gimple-classes-v2-option-3 git branch, having bootstrappedregrested it on x86_64-unknown-linux-gnu (Fedora 20)]. This is the the 89-patch kit from https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01148.html with various fixups, reviewed by Jeff and Richi. This strengthens the type requirements on numerous places in the middle end. For example, the callgraph now works on gcall * rather than plain gimple, capturing the GIMPLE_CALL requirement and checking that at compile-time. This gives us earlier failures when bugs occur, closer to the failure point, and (I hope) more readable code (since the assumptions made become clearer). It converts about half of the gimple_foo_ accessors, so that they take a gimple subclass pointer. [In theory we could add a gimple-compat.h that adds back overloads for these if you have out-of-tree code that uses the accessors with a plain gimple]. I've dropped about 80 followup patches where I attempted to convert the rest of the accessors, which led to contorted and overly-verbose C++isms in the code. Discussion on IRC indicates unhappiness with the cut-down version, a feeling that it's still too verbose. I'm sorry about that. There may be ways of shortening the API (if that's acceptable at this dev stage). Two common patterns relating to gimple_statement_iterator: (A) after detecting a particular kind of statement currently visited by a gsi, call some function, passing it the gsi, where the callee has something like this (in what I merged): - gimple stmt = gsi_stmt (*gsi); + greturn *stmt = as_a greturn * (gsi_stmt (*gsi)); i.e. where we then rely on it being a GIMPLE_RETURN in the callee. Would it be useful to have a less verbose spelling of, say: as_a greturn * (gsi_stmt (*gsi)); ? (B) a check for a particular gimple code on the gsi_stmt e.g. like this in what I merged: - if (gimple_code (stmt) != GIMPLE_CALL) + stmt = dyn_cast gcall * (gsi_stmt (*gsi)); + if (!stmt) return; Should we add a less verbose spelling of: dyn_cast gcall * (gsi_stmt (*gsi)); ? It's important to have a clear distinction between the as_a and dyn_cast cases: the former is a simple cast in a release build, whereas the latter is a conditional that could return NULL (in both debug and release builds). If we're going for minimal typing, then a method is shorter than a function, since we wouldn't need to spell out the gsi twice e.g.: (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. Dave
Re: gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
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... Andrew
Re: gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
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. Andrew
Re: gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
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) .. Andrew
Re: gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
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
Re: gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
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.
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 ?
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);
gcc_checking_assert (gimple_call_builtin_p (call, BUILT_IN_NORMAL));
diff --git a/gcc/auto-profile.c b/gcc/auto-profile.c
index 7055c4a..c6a17d7 100644
--- a/gcc/auto-profile.c
+++ b/gcc/auto-profile.c
@@ -1433,7 +1433,7 @@ afdo_vpt_for_early_inline (stmt_set *promoted_stmts)
for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (gsi))
{
-gcall *stmt = dyn_cast gcall * (gsi_stmt (gsi));
+gcall *stmt = dyn_cast gcall * (*gsi);
/* IC_promotion and early_inline_2 is done in multiple iterations.
No need to promoted the stmt if its in promoted_stmts (means
it is already been promoted in the previous iterations). */
diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
index 45c13b4..15b7c5c 100644
--- a/gcc/cfgexpand.c
+++ b/gcc/cfgexpand.c
@@ -2015,7 +2015,7 @@ label_rtx_for_bb (basic_block bb ATTRIBUTE_UNUSED)
{
glabel *lab_stmt;
- lab_stmt = dyn_cast glabel * (gsi_stmt (gsi));
+ lab_stmt = dyn_cast glabel * (*gsi);
if (!lab_stmt)
break;
@@ -4985,7 +4985,7 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls)
if (!gsi_end_p (gsi)
gimple_code (gsi_stmt (gsi)) == GIMPLE_RETURN)
{
- greturn *ret_stmt = as_a greturn * (gsi_stmt (gsi));
+ greturn *ret_stmt = as_a greturn * (*gsi);
gcc_assert (single_succ_p (bb));
gcc_assert (single_succ (bb) == EXIT_BLOCK_PTR_FOR_FN (cfun));
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index
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 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.
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
Re: gimple-classes-v2-option-3 git branch committed to svn trunk as r217787
On 11/19/2014 05:28 PM, David Malcolm wrote:
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 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.
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
I think the problem is different people will naturally think they should
do different things, and I see the logic to both.
In my various tree experiments, for a while I used the user defined
conversion as a dyn_cast because that is what seemed right to me. I
can assure you that can be flawed... mostly because if its something you
didn't anticipate, you just get a NULL back and things proceed... in
particular when calling functions. When I disabled that
auto-conversion, I discovered a whole bunch of code that was silently wrong.
At least with as_a you will catch it
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 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. 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
