| Message ID | 1416436087.3562.81.camel@surprise |
|---|---|
| State | New |
| Headers | show |
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 <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 > 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 immediately during compilation if things are wrong... but it still feels a little wrong to me. Likewise you could overload the operator= to do similar things but you wouldn't get unexpected auto-conversions as function parameters. SInce there are multiple ways of viewing it, I think its probably better to be explicit somehow. Andrew PS. this whole area sounds like a "best practices" decent C++ projects probably figured out a decade ago... or at least should have :-)
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 */