Re: [Patch, Fortran, OOP] PR 49638: [OOP] length parameter is ignored when overriding type bound character functions with constant length.

[Mikael Morin <mikael dot morin at sfr dot fr>]

On Thursday 04 August 2011 23:42:11 Janus Weil wrote:
> Hi all,
> 
> attached is a draft patch fixing the PR in the subject line and
> extending the checks for overriding type-bound functions. It regtests
> cleanly on x86_64-unknown-linux-gnu already, but I would like to have
> some feedback.
Some quick comments: 

> 
> The patch is rather large, but most of it is just mechanical, due to
> the fact that I added an extra argument to 'gfc_dep_compare_expr'. 
You might want to make the flag an implementation detail, that is keep the 
gfc_dep_compare_expr interface unchanged, but make the function a wrapper 
around the real function with the flag.

> I use this function to compare the string-length expressions of a
> character-valued TBP and an overriding procedure (the standard
> requires them to be equal). Inside 'gfc_dep_compare_expr' I had to add
> a minor piece to correctly respect commutativity of the multiplication
> operator (for the addition operator this was done already). 
OK

> The extra
> argument controls whether we check variable symbols for equality or
> just their names. For the overriding checks it is sufficient to check
> for names, because the arguments of the overriding procedure are
> required to have the same names as in the base procedure.

Unless you extend the flag thing to all the children function of 
gfc_dep_compare_expr (there are zillions of them), it is preferable IMO to 
make the diagnostic a warning, as identical expressions could be missed.

> 
> Moreover I extended the type check in 'check_typebound_override' to
> also check for correct rank, via 'compare_type_rank' instead of
> 'gfc_compare_types'. However, the former was local to interface.c, so
> I made it public (and should probably also rename it to gfc_...),
Yes

> or should one rather move 'check_typebound_override' to interface.c
> itself? I think it fits in there pretty nicely. After all it is
> checking the interfaces of overriding procedures.
Makes sense too. Either is fine.

> 
> Anything else missing for this patch? Or is it ok for trunk? (I will
> add corresponding test cases and a ChangeLog, of course.)
Apart for the error/warning change and the missing tests and ChangeLog, the 
patch is OK.

Mikael