[Fortran, Patch] Proposed type-bound procedures patch, part 1

[Daniel Kraft]

Hi Tobias et al.,

here's an updated version of my patch, incorporating your comments below 
and new logic to check if overriding is allowed based on the procedures' 
attributes and formal arglists.

Except the minor changes for the comments, new is 
check_typebound_override in resolve.c and the corresponding 
typebound_proc_6.f03 test; the other parts were untouched.

This patch does no full checking for correct function result type and 
argument type characteristics, see the two FIXME's in the code.  I will 
open a PR for this issue when committing and will work on this when I 
find time.

Regression test on GNU/Linux-x86-32 running.  Ok to commit when no 
regressions?

My comments:

> +  /* XXX: Below is quite a lot of ugly code duplication...  I couldrewrite this
> +     using macros, but that might be even uglier...  What do you think? Any
> +     completely other suggestions maybe?  */
> 
> I don't have any good idea. For procedure pointer components, one has
> the same problem. (Actually, match_binding_attributes can be used for
> both, (NO)PASS and PUBLIC/PRIVATE are the same; proc. pointers need POINTER
> while type-bounds procedures have DEFERRED and NON_OVERRIDABLE.)

I've removed the comment, we can think about clean-up and refactoring later.

> You could add DEFERRED to match_binding_attributes (or add a TODO).
> [I would go for the latter.]

I added code to match DEFERRED and print an unsupported error.

> +  /* XXX: Here's nothing about the need to be inside the specification part of
> +     a module!  */
> 
> True. There is only something about module procedures or external
> procedures.

Fixed the check in resolve_typebound_proc to handle this correctly.

> +  /* TODO:  Implement PROCEDURE(interface).  */
> 
> Aha, that explains why PROCEDURE(foo) gives strange error messages.
> 
> Maybe one should add an  if(match(" (")) gfc_error
> with: Procedure with interface only allowed in abstract types"
> That gives a nice error message, which is valid as long as abstract
> types are not supported.

Done.

> +  /* XXX: Should we use static array of GFC_MAX_SYMBOL_LENGTH+1 characters?
> +     This makes life easier to avoid leaking but may cost a lot of memory if
> +     there are lots of these structs around.  */
> 
> I think either is OK - I don't have strong preferences for either.

I stayed with the pointer as I like this slightly better.

> +/* XXX: Which file to put this best in?  */
> +gfc_symtree* gfc_find_typebound_proc (gfc_symbol*, const char*);
> 
> Spontaneously I would say: symbol.c, but resolve.c is also OK.

Moved together with the find-supertype method to symbol.c.

> +  if (!proc->attr.subroutine && !proc->attr.function)
> +    {
> +      gfc_error ("Binding-target '%s' must be a procedure at %L",
> +		 proc->name, &where);
> +      goto error;
> +    }
> 
> The error message is misleading for:
>   procedure() :: foo
>   [...]
>   type t
>   contains
>   procedure,nopass :: foo
> Here, foo is a procedure - but without an explicit interface and
> without knowing whether it is a subroutine or a function. How about:
> 
> Additionally, I find "binding-target" not that easy to understand. How
> about:
>   "Error: Procedure must be a module procedure or an external
>    procedure with an explicit interface"

Changed to this one.

> +	      gfc_error ("Binding-target '%s' with PASS(%s) at %L has no"
> +	      gfc_error ("Binding-target '%s' with PASS at %L must have at"
> +	    gfc_error ("Argument '%s' of the binding target '%s' with PASS(%s)"
> 
> Ditto: I still do not like "Binding-target" - how about "Procedure" ?

Changed.

> +! XXX: Do overriding bindings have to have the same or some "matching" signature
> +! as their overridden binding?
> 
> Yes, see long list in "4.5.6.2 Type-bound procedure overriding".

See resolve.c:check_typebound_override for my implementation of this 
section.

> +    ! XXX: How to do a PRIVATE subroutine for access checking?
> 
> You don't mean something like the following, do you? Without parsing the
> CALL statement, it is a bit difficult to test.

No, I meant something like

MODULE somemod
CONTAINS
   PRIVATE SUBROUTINE foo () ! If something like this is possible
   END SUBROUTINE foo
END MODULE somemod

...

TYPE t
CONTAINS
   PROCEDURE, NOPASS :: foo ! Error, foo is not accessible
END TYPE t

Is this possible and if so, how?  Then I'll add a check for this case.

> +  /* Find the super-type of the current derived type.  */
> +  /* XXX: Should we do this one time and make super_type a global, too?
> */
> +  super_type = get_derived_super_type (resolve_bindings_derived);
> 
> I don't have a strong opinion on this. It is a question of speed vs.
> complexity/readability. I'm not convinced that it will help much with
> the calculation speed. (And I have to admit I'm not too concerned about
> the compilation speed - unless something becomes really a hot spot.)

Left it as it was, I think this is clearer.

> Can you add additional checks for
> (a) procedures which are abstract interfaces [like "three"]
> (b) neither module nor external procedures   [like "bar"]
> see my previous email with the example at
> http://gcc.gnu.org/ml/fortran/2008-08/msg00150.html

Done, those should work.

> Otherwise OK if you add a not-yet-implemented error.
> Thanks for the patch.

Added the error.

Thanks for the review!
Daniel

-- 
Done:     Arc-Bar-Cav-Sam-Val-Wiz, Dwa-Elf-Gno-Hum-Orc, Law-Neu-Cha, Fem-Mal
To go:    Hea-Kni-Mon-Pri-Ran-Rog-Tou
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch.changelog
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20080824/a85dbf73/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: patch.diff
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20080824/a85dbf73/attachment-0001.ksh>