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