[patch, fortran] PR32432 SEGV/endless loop after: "ERROR: ... already is initialized"

[Jerry DeLisle]

Janne Blomqvist wrote:
> :REVIEWMAIL:
> 
> On 7/3/07, Jerry DeLisle <jvdelisle@verizon.net> wrote:
>> OK for trunk?
> 
> Yes. A few minor issues below though.
> 
> *************** gfc_assign_data_value (gfc_expr *lvalue,
> *** 293,300 ****
>            gfc_error ("'%s' at %L already is initialized at %L",
>               lvalue->symtree->n.sym->name, &lvalue->where,
>               &init->where);
> !           gfc_free_expr (init);
> !           init = NULL;
>          }
> 
>        if (init == NULL)
> --- 295,301 ----
>            gfc_error ("'%s' at %L already is initialized at %L",
>               lvalue->symtree->n.sym->name, &lvalue->where,
>               &init->where);
> !           return FAILURE;
>          }
> 
> Won't this create a memory leak as you are no longer freeing init?

No, to enter into this condition, init is not NULL.

       if (init == NULL)
	expr = gfc_get_expr ();
       else
	expr = init;

So gfc_get_expr () is never called.  expr is derived from the calling function. 
  This is why I believe the segfault was occurring.

> 
> *************** gfc_assign_data_value (gfc_expr *lvalue,
> *** 423,428 ****
> --- 424,431 ----
>      symbol->value = expr;
>    else
>      last_con->expr = expr;
> +
> +   return t;
>  }
> 
> Why not get rid of the t variable and just "return SUCCESS;", since
> you never actually use the t variable for anything beyond initially
> setting it to SUCCESS?

OK

> 
> Similarly in resolve.c,
> 
> !       t = gfc_assign_data_value (var->expr, values.vnode->expr, offset);
> !       if (t == FAILURE)
> !         break;
> 
> Why not just
> 
> if (gfc_assign_data_value (...) == FAILURE)
>    break;

We have to break out of a loop and do some cleanup of mpz variables before 
exiting and we must return t in this case. t is preset to SUCCESS.

> 
> Finally, please use unified diffs in the future; I suspect I'm not the
> only one who has difficulties following context diffs.
> 

I get this comment about unified diffs vs context diffs from time to time.  It 
was my understanding that context diffs are the gcc norm for patch submittals. 
Maybe I have this misunderstood?

Thanks for review.

Regards,

Jerry