Re: [PATCH, Fortran] Parse FINAL procedure declarations

[Daniel Kraft <d at domob dot eu>]

Tobias Burnus wrote:
> 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.

Thanks for those!

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

I wanted to wait until everything else was done, but I can integrate 
this for the next patch I send in.

> 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);

Honestly, I didn't know how to do this and just found out my code 
handled it like that...  But thanks for the code, I'll try to integrate 
this.  Of course I also like it better to have the error message correct ;)

> 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.

Will be changed.

> 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

Ok, I'll remove the XXX comment.

> 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.)

Here I was also not sure what exactly to do, that's why there's the XXX 
in; I just copied from module-procedure parsing and adapted until the 
tests worked reasonably.

I'll add the test case and try to integrate the code.

> 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.

I added the comment as "I'm not sure whether we should do this"--so it's 
up to you (Paul Thomas suggested this and I think it should work, but to 
me this looks a bit hackish/ugly so I left it open).  Please decide and 
then I will eventually change it; but I want to do it in this first part 
if we do it at all (so no 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"

Ok.

> 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.

Ok.  I use XXX for things I want to be discussed/clarified before 
check-in as there are no other XXX's ;).  So I'll change it to TODO, ok?

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

Oops, I'll fix.

Thanks for the comments!

Daniel

--
Done:     Bar-Sam-Val-Wiz, Dwa-Elf-Hum-Orc, Cha-Law, Fem-Mal
Underway: Ran-Gno-Neu-Fem
To go:    Arc-Cav-Hea-Kni-Mon-Pri-Rog-Tou