Re: [PATCH] fix for PR 59825

[Jakub Jelinek <jakub at redhat dot com>]

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

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.

	Jakub