RE: [PATCH] fix for PR 59825

["Iyer, Balaji V" <balaji dot v dot iyer at intel dot com>]

> -----Original Message-----
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Thursday, January 23, 2014 5:28 AM
> To: Iyer, Balaji V
> Cc: gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH] fix for PR 59825
> 
> On Thu, Jan 23, 2014 at 10:27:58AM +0100, Jakub Jelinek wrote:
> > On Mon, Jan 20, 2014 at 10:40:31PM +0000, Iyer, Balaji V wrote:
> > > --- a/gcc/c/c-array-notation.c
> > > +++ b/gcc/c/c-array-notation.c
> > > @@ -1218,22 +1218,22 @@ fix_return_expr (tree expr)
> > >    return new_mod_list;
> > >  }
> > >
> > > -/* Walks through tree node T and find all the call-statements that do not
> return
> > > -   anything and fix up any array notations they may carry.  The return
> value
> > > -   is the same type as T but with all array notations replaced with
> appropriate
> > > -   STATEMENT_LISTS.  */
> > > +/* Callback for walk_tree.  Expands all array notations in *TP.
> *WALK_SUBTREES
> > > +   is set to 1 unless *TP contains no array notation expressions.
> Parameter
> > > +   D is unused.  */
> > >
> > > -tree
> > > -expand_array_notation_exprs (tree t)
> > > +static tree
> > > +expand_array_notations (tree *tp, int *walk_subtrees, void *d
> > > +ATTRIBUTE_UNUSED)
> >
> > As we are now using C++, just use
> > expand_array_notations (tree *tp, int *walk_subtrees, void *) and
> > remove the comment about unused parameter D.
> >
> > >  {
> > > -  if (!contains_array_notation_expr (t))
> > > -    return t;
> > > +  if (!contains_array_notation_expr (*tp))
> >
> > Why do you want to walk the whole subtree once more at every level?
> > That has bad time complexity for very deep trees.
> > Can't you just do that in expand_array_notations_exprs once?
> 
> Though, as this is not a regression with this patch and the patch fixes a bug in
> the testsuite, perhaps you can do that incrementally.
> 
> So, can you please change the comment and expand_array_notations
> prototype, then commit (patch preapproved)?
> 

Ok. I will commit the patch with the fix you suggested above. For your reference, I have attached a patch.

Thanks,

Balaji V. Iyer.


> Then please add to todo list that walking all subtrees twice at every level is
> something you should avoid.  Perhaps for the trees you don't explictly handle
> you could gather in *(bool *)d whether it contained any array notations, and
> trees you actually handle could first walk the subtrees manually to gather the
> flags and transform what needs to be transformed in there, and then just
> based on the bool flag decide if it should do anything and what.  Also, how is
> e.g. array notation supposed to be handled in case of nested MODIFY_EXPRs
> where the nested MODIFY_EXPRs contain array notations?  I mean
> something like:
> a[0:10] = (b[0:10] ? (c[0:10] = d[0:10] + (e[0:10] = f[0:10] ? (g[0:10] * 3 : g[0:10]
> + 12))) : 5); etc.?
> I'd say contains_array_notation_expr should also be rewritten to use
> walk_tree, so should be the C++ AN expansion etc.  Just you'll need to use
> cp_walk_tree for C++ probably and thus the c-family/ code should just
> contain callbacks that you can use for the walking, not the actual calls to
> walk_tree/cp_walk_tree.
> 

Ok. I will look into this..

> 	Jakub

Attachment: diff.txt
Description: diff.txt