Re: [Fortran] PR37336 - FIINAL patch [1/n]: Implement the finalization wrapper subroutine

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

Mikael Morin wrote:
> On 29/08/2012 21:53, Tobias Burnus wrote:
> > a) The main patch, which implements the wrapper.
> >    I am asking for approval for that patch.
> A few more nitpicks below.
>
> > I would like to include the patch (c) as modifying the vtable changes
> > the ABI. Bumping the .mod version is a reliable way to force
> > recompilation. The alternative is to wait until the final FINAL patch
> > before bumping the .mod version (and disable the "_final" generation).
> I don't like bumping the module version, for something not
> module-related (vtypes are output as normal types in the module files),
> but I guess it is the most user-friendly thing to do.

I also do not like it - but it might otherwise lead to segfaults at run 
time (or the wrong procedure being called), which is even uglier.

> > Is the patch (a) OK for the trunk? With which version of (c)?
> >
> > (I am slightly inclined to do the .mod bump now. As a follow up, one can
> > also commit Janus' proc-pointer patch,
> > http://gcc.gnu.org/ml/fortran/2012-04/msg00033.html, though I think
> > someone has still to review it.)
> I am inclined to disable finalization completely (thus defer .mod bump),
> because the new code is hardly tested and doesn't provide (yet) new
> benefits such as reduced memory leaks as it is disabled.
> We could do the bump now, but I'm afraid that we discover a bug when
> finalization gets completely implemented, and we have to bump again to
> fix it (though I don't see what kind of bug it could be).

I concur.

> I think Janus' patch is a much less serious problem in the sense that
> people trying to link codes compiled with patched and non-patched
> compiler will get a link error.  I don't think it's worth a .mod bump.

Well, the idea was: If we do bump the .mod for this patch, having around 
that time Janus' patch (which also breaks the ABI) makes sense. I concur 
that his ABI breakage is less severe.

> For (a), I noticed a few indenting issues (8+ spaces) and (mostly
> wording) nits below to be fixed.  Then OK.

Fixed.

> > +/* Returns true if any of its nonpointer nonallocatable components or
> > +   their nonpointer nonallocatable subcomponents has a finalization
> > +   subroutine.  */
> > +
> > +static bool
> > +has_finalizer_component (gfc_symbol *derived)
> Rename to has_finalizable_component ?

I prefer finalizer because also allocatable components are finalizable 
but they are excluded by this check.

> > +/* Call DEALLOCATE for the passed component if it is allocatable, if it is
> > +   neither allocatable nor a pointer but has a finalizer, call it. If it
> > +   is a nonpointer component with allocatable or finalizes components, walk
> s/finalizes/finalizable/ ?

Actually: with allocatable components or has finalizers.

> > +   them. Either of the is required; other nonallocatables and pointers aren't
> s/the/them/ ?
done.

> > +   handled gracefully.
> > +   Note: The DEALLOCATE handling takes care of finalizers, coarray
> > +   deregistering and allocatable components of the allocatable.  */
> "coarray deregistering and allocatable components" is confusing.
>
> Note: If the component is allocatable, the DEALLOCATE handling takes
> care of calling the appropriate finalizer(s), of coarray deregistering,
> and of deallocating allocatable (sub)components.

Done.

> > +/* Generate the wrapper finalization/polymorphic freeing subroutine for the
> Difficult to read.
> "Generate the finalization/polymorphic freeing wrapper subroutine..." or
> something ?

Done.

> > +  /* We now create a wrapper, which does the following:
> > +     1. It calls the suitable finalization subroutine for this type
> s/It calls/Call/ ? (to be in line with the other verbs).
> > +     2. In a loop over all noninherited allocatable components and noninherited
> s/In a loop/Loop/

Done.


Thanks for the review. I have no committed it as Rev. 190869

Tobias