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

[Andrew MacLeod]

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 :-)