Re: [PATCH, Fortran] Parse FINAL procedure declarations

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

Hi Daniel,

Daniel Kraft wrote:
> I'm not sure about how reviewing is going on
Slowly - as real life interferes.

Regarding the patch: I think it is mostly OK, but I still have some remarks.


a) You should add towards the end of "gfc_match_final_decl" a
gfc_error with a not-imlemented-in-gfortran error message.


b) You should check whether the module procedure exists. Currently one 
gets for:

   final :: final2
                 1
Error: FINAL procedure at (1) must have exactly one argument

Which is misleading if the procedure does not exist. How about using 
simply at the very top of gfc_resolve_finalizers:

if (!list->procedure->attr.subroutine)
 gfc_error ("FINAL procedure '%s' at %L is not a subroutine", 
list->procedure->name, &list->where);

(This is how it is in the spririt of "[module] procedure" in generic 
interfaces. The function check at the very bottom can then be removed.)


For the argument checking such as OPTIONAL, INTENT etc. one should use 
&arg->declared_at rather than &list->where --- this gives a better error 
location.


c)
+  /* Construct the f2k_derived namespace if it is not yet there.  */
+  /* XXX: Set NULL as parent namespace?  */
+  if (!sym->f2k_derived)
+    sym->f2k_derived = gfc_get_namespace (NULL, 0);

I think that is OK


d)
+  module_ns = gfc_current_ns;
+  for (; module_ns; module_ns = module_ns->parent)
+    if (module_ns->proc_name->attr.flavor == FL_MODULE)
+      break;
+
+  if (module_ns == NULL)
+    {
+      /* XXX: What's about this?  */
+      gfc_error ("Derived type declaration with FINAL must be inside a 
MODULE");

That looks wrong. I think the derived type-declaration with FINAL 
attribute need to be in the *specification part* of a module. For 
instance the following example is wrong. (It is rejected, but much later 
with a suboptimal error message.)

[Reasoning from the standard: finalization subroutine needs to be a 
module procedure (and thus not an internal procedure) and as the derived 
type needs to be passed, it has to be declared before.]

Invalid test case:

module m
contains
subroutine bar
 type t
 contains ! << OK
   final :: final2 ! << FINAL is INVALID in this context
 end type t
contains
  subroutine final2(x)
   type(t) :: x
  end subroutine final2
end subroutine bar
end module m


How about something similar to:

if (!gfc_state_stack->previous | | gfc_state_stack->previous->state != 
COMP_MODULE)
 gfc_error ("Derived type declaration with FINAL must be in the 
specification part of a module");

(Completely untested, I might miss another layer because of contains.)



e)
+  /* The namespace containing type-associated procedure symbols.  */
+  /* XXX: Make this union with formal.  */
+  struct gfc_namespace *f2k_derived;

Will you do this for the follow up patch? In any case if you intent to 
commit the patch before you fixed it, please change XXX to TODO.


f)
+   been defined and we now know their defined arguments, check that 
they fulfill
+   the requirements by the standard to procedures used as finalizers.  */

"requirements of the standard", "for procedures"



g)
+      /* XXX: Kind parameters once they are implemented.  */

Better use either TODO (or FIXME); we currently have 14 FIXMEs and 107 
TODOs. One can easily grep for those, but if one adds other items such 
as XXX it gets more and more difficult. And please add a verb at the 
beginning of the sentence.


h) The following is invalid but not detected:
 type t
   contains
   contains ! << INVALID
 end type t


Tobias