diff mbox

[fortran] use vec<> in frontend-passes.c

Message ID 53F919F5.2050602@netcologne.de
State New
Headers show

Commit Message

Thomas Koenig Aug. 23, 2014, 10:47 p.m. UTC
Hello world,

the attached patch uses the vec<> templates instead of the
home-grown memory management.

Did I get the style right?  I was a bit confused because, with
the declarations used in the patch, using the vec_safe_truncate
function failed with a failure to find the right template.
I'm not an experienced C++ programmer, so I would appreciate a quick
look at what I did.

Regression-tested.  No test case because there is no new
functionality.

OK for trunk?  Any other comments?

	Thomas

2014-08-24  Thomas Koenig  <tkoenig@gcc.gnu.org>

        * frontend_passes (expr_array):  Replace by vec template.
        (expr_size):  Remove.
        (expr_count):  Remove.
        (doloop_list):  Replace by vec template.
        (doloop_size):  Remove.
        (gfc_run_passes):  Adjust to use of vec template.
        (cfe_register_funcs):  Likewise.
        (cfe_expr_0):  Likewise.
        (doloop_code):  Likewise.

Comments

Trevor Saunders Aug. 24, 2014, 3:02 a.m. UTC | #1
On Sun, Aug 24, 2014 at 12:47:17AM +0200, Thomas Koenig wrote:
> Hello world,
> 
> the attached patch uses the vec<> templates instead of the
> home-grown memory management.
> 
> Did I get the style right?  I was a bit confused because, with
> the declarations used in the patch, using the vec_safe_truncate
> function failed with a failure to find the right template.
> I'm not an experienced C++ programmer, so I would appreciate a quick
> look at what I did.
> 
> Regression-tested.  No test case because there is no new
> functionality.
> 
> OK for trunk?  Any other comments?
> 
> 	Thomas
> 
> 2014-08-24  Thomas Koenig  <tkoenig@gcc.gnu.org>
> 
>         * frontend_passes (expr_array):  Replace by vec template.
>         (expr_size):  Remove.
>         (expr_count):  Remove.
>         (doloop_list):  Replace by vec template.
>         (doloop_size):  Remove.
>         (gfc_run_passes):  Adjust to use of vec template.
>         (cfe_register_funcs):  Likewise.
>         (cfe_expr_0):  Likewise.
>         (doloop_code):  Likewise.

> Index: frontend-passes.c
> ===================================================================
> --- frontend-passes.c	(Revision 214281)
> +++ frontend-passes.c	(Arbeitskopie)
> @@ -50,8 +50,7 @@ static int count_arglist;
>  /* Pointer to an array of gfc_expr ** we operate on, plus its size
>     and counter.  */
>  
> -static gfc_expr ***expr_array;
> -static int expr_size, expr_count;
> +static vec<gfc_expr **> expr_array = vec<gfc_expr **>();

that's usually written as just static vec<T> foo; vec doesn't actually
have ctors, and 0 initialization works fine.

>  /* Pointer to the gfc_code we currently work on - to be able to insert
>     a block before the statement.  */
> @@ -81,9 +80,10 @@ static int iterator_level;
>  
>  /* Keep track of DO loop levels.  */
>  
> -static gfc_code **doloop_list;
> -static int doloop_size, doloop_level;
> +static vec<gfc_code *> doloop_list = vec<gfc_code *>();

same

> @@ -101,23 +101,19 @@ gfc_run_passes (gfc_namespace *ns)
>    /* Warn about dubious DO loops where the index might
>       change.  */
>  
> -  doloop_size = 20;
>    doloop_level = 0;
> -  doloop_list = XNEWVEC(gfc_code *, doloop_size);
> +  //  doloop_list = ;

what is this comment supposed to mean?

>    doloop_warn (ns);
> -  XDELETEVEC (doloop_list);
> +  doloop_list.truncate (0);

.release () would be more typical.

>  
>    if (gfc_option.flag_frontend_optimize)
>      {
> -      expr_size = 20;
> -      expr_array = XNEWVEC(gfc_expr **, expr_size);
> -
>        optimize_namespace (ns);
>        optimize_reduction (ns);
>        if (gfc_option.dump_fortran_optimized)
>  	gfc_dump_parse_tree (ns, stdout);
>  
> -      XDELETEVEC (expr_array);
> +      expr_array.truncate (0);

same

> @@ -608,36 +599,35 @@ cfe_expr_0 (gfc_expr **e, int *walk_subtrees,
>        return 0;
>      }
>  
> -  expr_count = 0;
> +  expr_array.truncate (0);

.release ()

>    gfc_expr_walker (e, cfe_register_funcs, NULL);
>  
>    /* Walk through all the functions.  */
>  
> -  for (i=1; i<expr_count; i++)
> +  for (i=1; expr_array.iterate (i, &ei); i++)

FOR_EACH_VEC_ELT, though Its not really clear to my why that's better
than
size_t length = vec.length ();
for (size_t i = 0; i < length; i++)
  // do something with vec[i]

>      {
>        /* Skip if the function has been replaced by a variable already.  */
> -      if ((*(expr_array[i]))->expr_type == EXPR_VARIABLE)
> +      if ((*ei)->expr_type == EXPR_VARIABLE)
>  	continue;
>  
>        newvar = NULL;
> -      for (j=0; j<i; j++)
> +      for (j=0; j<i && expr_array.iterate (j, &ej); j++)

the .iterate call is useless since j must be < i, and the vector is
at least i long right?
Trev
Steve Kargl Aug. 24, 2014, 7:45 a.m. UTC | #2
On Sun, Aug 24, 2014 at 12:47:17AM +0200, Thomas Koenig wrote:
> Hello world,
> 
> the attached patch uses the vec<> templates instead of the
> home-grown memory management.
> 
> Did I get the style right?  I was a bit confused because, with
> the declarations used in the patch, using the vec_safe_truncate
> function failed with a failure to find the right template.
> I'm not an experienced C++ programmer, so I would appreciate a quick
> look at what I did.
> 
> Regression-tested.  No test case because there is no new
> OK for trunk?  Any other comments?
> 

Does this fix a bug in gfortran?  "If it is not broken,
don't fix it".
diff mbox

Patch

Index: frontend-passes.c
===================================================================
--- frontend-passes.c	(Revision 214281)
+++ frontend-passes.c	(Arbeitskopie)
@@ -50,8 +50,7 @@  static int count_arglist;
 /* Pointer to an array of gfc_expr ** we operate on, plus its size
    and counter.  */
 
-static gfc_expr ***expr_array;
-static int expr_size, expr_count;
+static vec<gfc_expr **> expr_array = vec<gfc_expr **>();
 
 /* Pointer to the gfc_code we currently work on - to be able to insert
    a block before the statement.  */
@@ -81,9 +80,10 @@  static int iterator_level;
 
 /* Keep track of DO loop levels.  */
 
-static gfc_code **doloop_list;
-static int doloop_size, doloop_level;
+static vec<gfc_code *> doloop_list = vec<gfc_code *>();
 
+static int doloop_level;
+
 /* Vector of gfc_expr * to keep track of DO loops.  */
 
 struct my_struct *evec;
@@ -101,23 +101,19 @@  gfc_run_passes (gfc_namespace *ns)
   /* Warn about dubious DO loops where the index might
      change.  */
 
-  doloop_size = 20;
   doloop_level = 0;
-  doloop_list = XNEWVEC(gfc_code *, doloop_size);
+  //  doloop_list = ;
   doloop_warn (ns);
-  XDELETEVEC (doloop_list);
+  doloop_list.truncate (0);
 
   if (gfc_option.flag_frontend_optimize)
     {
-      expr_size = 20;
-      expr_array = XNEWVEC(gfc_expr **, expr_size);
-
       optimize_namespace (ns);
       optimize_reduction (ns);
       if (gfc_option.dump_fortran_optimized)
 	gfc_dump_parse_tree (ns, stdout);
 
-      XDELETEVEC (expr_array);
+      expr_array.truncate (0);
     }
 }
 
@@ -420,13 +416,7 @@  cfe_register_funcs (gfc_expr **e, int *walk_subtre
 	return 0;
     }
 
-  if (expr_count >= expr_size)
-    {
-      expr_size += expr_size;
-      expr_array = XRESIZEVEC(gfc_expr **, expr_array, expr_size);
-    }
-  expr_array[expr_count] = e;
-  expr_count ++;
+  expr_array.safe_push (e);
   return 0;
 }
 
@@ -599,6 +589,7 @@  cfe_expr_0 (gfc_expr **e, int *walk_subtrees,
 {
   int i,j;
   gfc_expr *newvar;
+  gfc_expr **ei, **ej;
 
   /* Don't do this optimization within OMP workshare. */
 
@@ -608,36 +599,35 @@  cfe_expr_0 (gfc_expr **e, int *walk_subtrees,
       return 0;
     }
 
-  expr_count = 0;
+  expr_array.truncate (0);
 
   gfc_expr_walker (e, cfe_register_funcs, NULL);
 
   /* Walk through all the functions.  */
 
-  for (i=1; i<expr_count; i++)
+  for (i=1; expr_array.iterate (i, &ei); i++)
     {
       /* Skip if the function has been replaced by a variable already.  */
-      if ((*(expr_array[i]))->expr_type == EXPR_VARIABLE)
+      if ((*ei)->expr_type == EXPR_VARIABLE)
 	continue;
 
       newvar = NULL;
-      for (j=0; j<i; j++)
+      for (j=0; j<i && expr_array.iterate (j, &ej); j++)
 	{
-	  if (gfc_dep_compare_functions (*(expr_array[i]),
-					*(expr_array[j]), true)	== 0)
+	  if (gfc_dep_compare_functions (*ei, *ej, true) == 0)
 	    {
 	      if (newvar == NULL)
-		newvar = create_var (*(expr_array[i]));
+		newvar = create_var (*ei);
 
 	      if (gfc_option.warn_function_elimination)
-		warn_function_elimination (*(expr_array[j]));
+		warn_function_elimination (*ej);
 
-	      free (*(expr_array[j]));
-	      *(expr_array[j]) = gfc_copy_expr (newvar);
+	      free (*ej);
+	      *ej = gfc_copy_expr (newvar);
 	    }
 	}
       if (newvar)
-	*(expr_array[i]) = newvar;
+	*ei = newvar;
     }
 
   /* We did all the necessary walking in this function.  */
@@ -1671,25 +1661,24 @@  doloop_code (gfc_code **c, int *walk_subtrees ATTR
   int i;
   gfc_formal_arglist *f;
   gfc_actual_arglist *a;
+  gfc_code *cl;
 
   co = *c;
 
+  /* If the doloop_list grew, we have to truncate it here.  */
+
+  if ((unsigned) doloop_level < doloop_list.length())
+    doloop_list.truncate (doloop_level);
+
   switch (co->op)
     {
+
     case EXEC_DO:
 
-      /* Grow the temporary storage if necessary.  */
-      if (doloop_level >= doloop_size)
-	{
-	  doloop_size = 2 * doloop_size;
-	  doloop_list = XRESIZEVEC (gfc_code *, doloop_list, doloop_size);
-	}
-
-      /* Mark the DO loop variable if there is one.  */
       if (co->ext.iterator && co->ext.iterator->var)
-	doloop_list[doloop_level] = co;
+	doloop_list.safe_push (co);
       else
-	doloop_list[doloop_level] = NULL;
+	doloop_list.safe_push ((gfc_code *) NULL);
       break;
 
     case EXEC_CALL:
@@ -1708,14 +1697,14 @@  doloop_code (gfc_code **c, int *walk_subtrees ATTR
 
       while (a && f)
 	{
-	  for (i=0; i<doloop_level; i++)
+	  for (i=0; doloop_list.iterate (i, &cl); i++)
 	    {
 	      gfc_symbol *do_sym;
 	      
-	      if (doloop_list[i] == NULL)
+	      if (cl == NULL)
 		break;
 
-	      do_sym = doloop_list[i]->ext.iterator->var->symtree->n.sym;
+	      do_sym = cl->ext.iterator->var->symtree->n.sym;
 	      
 	      if (a->expr && a->expr->symtree
 		  && a->expr->symtree->n.sym == do_sym)
@@ -1968,7 +1957,6 @@  gfc_code_walker (gfc_code **c, walk_code_fn_t code
 
 	  switch (co->op)
 	    {
-
 	    case EXEC_BLOCK:
 	      WALK_SUBCODE (co->ext.block.ns->code);
 	      if (co->ext.block.assoc)