Message ID | 518CCCED.5080104@net-b.de |
---|---|
State | New |
Headers | show |
Hi Tobias, > GCC 4.7 added some additional checks for type-bound procedure overriding. > However, doing so it weakened the check whether the nonpass argument has the > same type. > > While for normal arguments, passing the parent type to an extended type is > fine, for overriding the type (of nonpass arguments) must be exactly the > same as in the original type. in principle I think your idea to tighten up the type check by symmetrizing it is ok. However, I'm not sure if the place where you do it is the most preferable: I think it should rather go inside of check_dummy_characteristics (since any check for the dummy characteristics should include the 'strict' type check, and not only a check of type compatibility, cf. F08:12.3.2.2). Anyway, there already is a call to 'compare_type_rank' in 'check_dummy_characteristics'. So, you could either just symmetrize this, or alternatively use a double 'gfc_compare_types' and a separate check for the rank. Moreover, I think your current patch misses e.g. the assumed-type handling that is present in compare_type_rank. Cheers, Janus
Hi Janus, Janus Weil wrote: > in principle I think your idea to tighten up the type check by > symmetrizing it is ok. I actually kind of copied it from the proc-pointer assignment check, which does the same. > However, I'm not sure if the place where you do it is the most > preferable: I think it should rather go inside of > check_dummy_characteristics (since any check for the dummy > characteristics should include the 'strict' type check, and not only a > check of type compatibility, cf. F08:12.3.2.2). Okay. Maybe one can at the same time replace the rank+type check by a type and a rank check to provide a better error message, i.e. show the type for a type mismatch (showing both types)* and the rank for a rank mismatch (showing both ranks). (* best with special handling of "no_arg_check") > Moreover, I think your current patch misses e.g. the assumed-type > handling that is present in compare_type_rank. Good point. Another reason to think about splitting the two. Would you interested updating this patch? Or should I put it again on my growing to-do list? Tobias
Hi, >> However, I'm not sure if the place where you do it is the most >> preferable: I think it should rather go inside of >> check_dummy_characteristics (since any check for the dummy >> characteristics should include the 'strict' type check, and not only a >> check of type compatibility, cf. F08:12.3.2.2). > > Okay. Maybe one can at the same time replace the rank+type check by a type > and a rank check to provide a better error message, i.e. show the type for a > type mismatch (showing both types)* and the rank for a rank mismatch > (showing both ranks). yes, actually I also thought that splitting it up might be a good idea. >> Moreover, I think your current patch misses e.g. the assumed-type >> handling that is present in compare_type_rank. > > Good point. Another reason to think about splitting the two. Right. However, the question is whether the patch will not become too involved for backporting. > Would you interested updating this patch? Or should I put it again on my > growing to-do list? My weekend is extremely tight, but I might find a quite hour in the evenings during the next week (unless you can beat me to it) ... Cheers, Janus
>>> However, I'm not sure if the place where you do it is the most >>> preferable: I think it should rather go inside of >>> check_dummy_characteristics (since any check for the dummy >>> characteristics should include the 'strict' type check, and not only a >>> check of type compatibility, cf. F08:12.3.2.2). >> >> Okay. Maybe one can at the same time replace the rank+type check by a type >> and a rank check to provide a better error message, i.e. show the type for a >> type mismatch (showing both types)* and the rank for a rank mismatch >> (showing both ranks). > > yes, actually I also thought that splitting it up might be a good idea. > > >>> Moreover, I think your current patch misses e.g. the assumed-type >>> handling that is present in compare_type_rank. >> >> Good point. Another reason to think about splitting the two. > > Right. > > However, the question is whether the patch will not become too > involved for backporting. Ok, so: How about the attached patch as a simple & backportable fix for the regression? (Ok for trunk/4.8/4.7?) I suggest the rest (i.e. splitting up compare_type_rank, improving error messages, etc) should be done in a follow-up patch on trunk only. (I might even volunteer do to this once I find the time ...) Cheers, Janus
>> in principle I think your idea to tighten up the type check by >> symmetrizing it is ok. > > I actually kind of copied it from the proc-pointer assignment check, which > does the same. Btw, I think you refer to this piece of code in expr.c (gfc_check_pointer_assign), right? if (!gfc_compare_interfaces (s1, s2, name, 0, 1, err, sizeof(err), NULL, NULL)) { gfc_error ("Interface mismatch in procedure pointer assignment " "at %L: %s", &rvalue->where, err); return false; } if (!gfc_compare_interfaces (s2, s1, name, 0, 1, err, sizeof(err), NULL, NULL)) { gfc_error ("Interface mismatch in procedure pointer assignment " "at %L: %s", &rvalue->where, err); return false; } Since this will ultimately rely on check_dummy_characteristics, too, it might even be possible to remove the duplication here, after my patch introduces the symmetrization on a deeper level ... ? Cheers, Janus
Janus Weil wrote: > Ok, so: How about the attached patch as a simple & backportable fix > for the regression? (Ok for trunk/4.8/4.7?) I think that part is okay - but as you mentioned TYPE(*) in your last email: That doesn't work; I think compare_type_rank should be made asymmetrical in this regard (ditto for "!gcc$ attributes no_arg_check"). Thus, could you fix that part as well? > I suggest the rest (i.e. splitting up compare_type_rank, improving > error messages, etc) should be done in a follow-up patch on trunk > only. (I might even volunteer do to this once I find the time ...) I am fine with that suggestion. Regarding the your other email in this thread: I was indeed talking about that part of the proc-pointer-assignment check. Tobias
Sorry for taking so long to come back to this (I was traveling all of last week) ... >> Ok, so: How about the attached patch as a simple & backportable fix for >> the regression? (Ok for trunk/4.8/4.7?) > > I think that part is okay - but as you mentioned TYPE(*) in your last email: > That doesn't work; I think compare_type_rank should be made asymmetrical in > this regard (ditto for "!gcc$ attributes no_arg_check"). Thus, could you fix > that part as well? What do you mean? That "type(t)" arguments should be able to override "type(*)", but not vice versa? I think that would interfere with the current patch, which symmetrizes the call to "compare_type_rank". If one direction gives a negative result, then both cases will be rejected. Anyway, anything in this direction is probably a non-regression and should rather be handled as a follow-up. Is the current patch (http://gcc.gnu.org/ml/fortran/2013-05/msg00045.html) ok for trunk/4.8/4.7? Cheers, Janus
Janus Weil wrote: >>> Ok, so: How about the attached patch as a simple & backportable fix for >>> the regression? (Ok for trunk/4.8/4.7?) >> I think that part is okay - but as you mentioned TYPE(*) in your last email: >> That doesn't work; I think compare_type_rank should be made asymmetrical in >> this regard (ditto for "!gcc$ attributes no_arg_check"). Thus, could you fix >> that part as well? > What do you mean? Try the four attached test cases - they should all be rejected, but they are accepted. > Anyway, anything in this direction is probably a non-regression and > should rather be handled as a follow-up. Is the current patch > (http://gcc.gnu.org/ml/fortran/2013-05/msg00045.html) ok for > trunk/4.8/4.7? OK. Still, I would like if the attached test cases would be rejected. (The first one only affects GCC 4.9, the others also GCC4.8). Tobias PS: If you have time, could you please review http://gcc.gnu.org/ml/fortran/2013-05/msg00081.html (dealloc intent(in) poly array)? (For the finalization wrapper patch, I will later send a follow-up patch which addresses some additional issues, I found while testing.)
>> Anyway, anything in this direction is probably a non-regression and >> should rather be handled as a follow-up. Is the current patch >> (http://gcc.gnu.org/ml/fortran/2013-05/msg00045.html) ok for >> trunk/4.8/4.7? > > OK. Thanks. Committed to trunk as r199375. Will do 4.8 and 4.7 soon. > Still, I would like if the attached test cases would be rejected. (The > first one only affects GCC 4.9, the others also GCC4.8). I'm leaving that for the follow-up (more of which was discussed upthread). I guess one can argue whether the first one should actually be rejected (the other two: certainly). > PS: If you have time, could you please review > http://gcc.gnu.org/ml/fortran/2013-05/msg00081.html (dealloc intent(in) poly > array)? Time is a rare commodity, but I'll see what I can do ... ;) Cheers, Janus
2013-05-10 Tobias Burnus <burnus@net-b.de> PR fortran/57217 * interface.c (gfc_check_typebound_override): Check whether nonpass types are identical same. 2013-05-10 Tobias Burnus <burnus@net-b.de> PR fortran/57217 * gfortran.dg/typebound_override_4.f90: New. * gfortran.dg/typebound_proc_6.f03: Update dg-error. diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c index 1b967fa..8f22e4c 100644 --- a/gcc/fortran/interface.c +++ b/gcc/fortran/interface.c @@ -4128,6 +4128,17 @@ gfc_check_typebound_override (gfc_symtree* proc, gfc_symtree* old) } check_type = proc_pass_arg != argpos && old_pass_arg != argpos; + if (check_type + && (!gfc_compare_types (&proc_formal->sym->ts, &old_formal->sym->ts) + || !gfc_compare_types (&old_formal->sym->ts, + &proc_formal->sym->ts))) + { + gfc_error ("Argument type mismatch for the overriding procedure " + "'%s' at %L: %s shall be %s", proc->name, &where, + gfc_typename (&proc_formal->sym->ts), + gfc_typename (&old_formal->sym->ts)); + return false; + } if (!check_dummy_characteristics (proc_formal->sym, old_formal->sym, check_type, err, sizeof(err))) { diff --git a/gcc/testsuite/gfortran.dg/typebound_override_4.f90 b/gcc/testsuite/gfortran.dg/typebound_override_4.f90 new file mode 100644 index 0000000..e6f9805 --- /dev/null +++ b/gcc/testsuite/gfortran.dg/typebound_override_4.f90 @@ -0,0 +1,36 @@ +! { dg-do compile } +! +! PR fortran/57217 +! +! Contributed Salvatore Filippone +! +module base_mod + type base_type + integer :: kind + contains + procedure, pass(map) :: clone => base_clone + end type base_type +contains + subroutine base_clone(map,mapout,info) + implicit none + class(base_type), intent(inout) :: map + class(base_type), intent(inout) :: mapout + integer :: info + end subroutine base_clone +end module base_mod + +module r_mod + use base_mod + type, extends(base_type) :: r_type + real :: dat + contains + procedure, pass(map) :: clone => r_clone ! { dg-error "Argument type mismatch for the overriding procedure 'clone' at .1.: CLASS.r_type. shall be CLASS.base_type." } + end type r_type +contains + subroutine r_clone(map,mapout,info) + implicit none + class(r_type), intent(inout) :: map + class(r_type), intent(inout) :: mapout + integer :: info + end subroutine r_clone +end module r_mod diff --git a/gcc/testsuite/gfortran.dg/typebound_proc_6.f03 b/gcc/testsuite/gfortran.dg/typebound_proc_6.f03 index 3a32cbc..1fe2580 100644 --- a/gcc/testsuite/gfortran.dg/typebound_proc_6.f03 +++ b/gcc/testsuite/gfortran.dg/typebound_proc_6.f03 @@ -89,7 +89,7 @@ MODULE testmod ! For corresponding dummy arguments. PROCEDURE, PASS :: corresp1 => proc_tmeint ! Ok. PROCEDURE, PASS :: corresp2 => proc_tmeintx ! { dg-error "should be named 'a'" } - PROCEDURE, PASS :: corresp3 => proc_tmereal ! { dg-error "Type/rank mismatch in argument 'a'" } + PROCEDURE, PASS :: corresp3 => proc_tmereal ! { dg-error "Argument type mismatch for the overriding procedure 'corresp3' at .1.: REAL.4. shall be INTEGER.4." } END TYPE t