Re: [Patch, Fortran] PR 41586: Allocatable _scalars_ are never auto-deallocated

[Tobias Burnus <burnus at net-b dot de>]

On 10/19/2009 04:50 PM, Janus Weil wrote:
> One aside, however: There is a strange thing in the dump for case 'c'
> (not necessarily connected to this patch):
>
>   struct t1 m;
>
>   m.j = 0B;
>   {
>     struct t1 D.1388;
>     struct t1 t1.0;
>
>     t1.0.j = 0B;
>     D.1388 = m;
>     m = t1.0;
>     if (D.1388.j != 0B)
>       __builtin_free ((void *) D.1388.j);
>     D.1388.j = 0B;
>   }
>
> If I'm not mistaken, this code was generated by "gfc_init_default_dt"
> to handle the default initialization of the variable 'm'. But it's not
> quite clear to me why we need two temporaries here, or why this
> 'D.1388' needs to be freed although it's barely used at all (and
> certainly not allocated). Maybe someone can enlighten me on this
> matter.
>   
I think one does not need two temporaries - maybe one could get rid of
one. (gfortran generates in my opinion too many temporaries. While in
principle -O3 should remove them, this does not always works as PR 41494
shows. Besides, in terms of dump readability and compile memory/time,
fewer temporaries are useful.)

What the code does is something like:
  if(allocated(m%j))  then
      deallocate(m%j) ! Free if already allocated
  end if
  nullify(m%j) ! apply default initializer
which makes sense if "m" is INTENT(OUT) in order not too leak memory.

> The patch was regtested on x86_64-unknown-linux-gnu. Ok for trunk?
>   

-  if (sym->ts.type == BT_DERIVED && sym->ts.u.derived->attr.alloc_comp)
+  /* Remember this variable for allocation/cleanup.  */
+  if (sym->attr.dimension || sym->attr.allocatable
+      || (sym->ts.type == BT_CLASS &&
+	  (sym->ts.u.derived->components->attr.dimension
+	   || sym->ts.u.derived->components->attr.allocatable)))
     gfc_defer_symbol_init (sym);
+  else if (sym->ts.type == BT_DERIVED && sym->ts.u.derived->attr.alloc_comp)
+    gfc_defer_symbol_init (sym);


I would prefer if you could add the condition of "else if" as "||" to
the first block:

+  if (sym->attr.dimension || sym->attr.allocatable
+      || (sym->ts.type == BT_CLASS &&
+	  (sym->ts.u.derived->components->attr.dimension
+	   || sym->ts.u.derived->components->attr.allocatable))
+      || (sym->ts.type == BT_DERIVED && sym->ts.u.derived->attr.alloc_comp))
     gfc_defer_symbol_init (sym);

I think that's more readable - otherwise one thinks that for BT_DERIVED
a different action will be done - and as another "||" does not require
more indention, I think it also does not decrease the readability.

Looks OK otherwise. Thanks for the patch!

Tobias

> 2009-10-19  Janus Weil  <janus@gcc.gnu.org>
>
> 	PR fortran/41586
> 	* parse.c (parse_derived): Correctly set 'alloc_comp' and 'pointer_comp'
> 	for CLASS variables.
> 	* trans-array.c (structure_alloc_comps): Handle deallocation and
> 	nullification of allocatable scalar components.
> 	* trans-decl.c (gfc_get_symbol_decl): Remember allocatable scalars for
> 	automatic deallocation.
> 	(gfc_trans_deferred_vars): Automatically deallocate allocatable scalars.
>
>
> 2009-10-19  Janus Weil  <janus@gcc.gnu.org>
>
> 	PR fortran/41586
> 	* gfortran.dg/auto_dealloc_1.f90: New test case.
>