Hi Tobias,

thanks for the review!

>> here is a patch for a rather old PR, which deals with correctness
>> checking for several cases, such as:
>> 1) dummy procedures
>> 2) proc-ptr assignments
>> 3) type-bound procedure overloading
>>
>> The patch adds a new function 'check_result_characteristics' to do
>> various checks on function results. This is largely analogous to
>> 'check_dummy_characteristics'. In both of them, there are still a few
>> attributes left to check, which I may add in a follow-up patch. Also I
>> had to disable the warning for cases where we can not finally
>> determine whether the string length or shape expressions are equal.
>> The treatment for those cases should be improved at some point, or one
>> should think about re-enabling the warnings.
>>
>> Regtested on x86_64-unknown-linux-gnu. Ok for trunk?
>
>
> Thanks for the patch. However, I wonder about:
>
> +  /* Check ALLOCATABLE attribute.  */
> +  if (r1->attr.allocatable != r2->attr.allocatable)
> +    {
> +      snprintf (errmsg, err_len, "ALLOCATABLE mismatch in function
> result");
> +      return FAILURE;
> +    }
>
> Shouldn't one check whether errmsg != NULL? That should only occur with
> generic resolution, but my impression is that that code is reached.

I have to admit that I did not properly think about this point. Now
that I do, I actually conclude that 'check_result_characteristics' is
apparently never called without errmsg. 'gfc_compare_interfaces' only
calls it when 'strict_flag' is set, and it is never called with
strict_flag and without errmsg.


> Additionally, I find "ALLOCATABLE mismatch" a bit unclear and would prefer
> something like "ALLOCATABLE attribute mismatch".

Ok, I have done so for this one and the other attribute checks.


> Other items to check for: CONTIGUOUS, type-length parameter (string length),
> procedure-pointer results.

The string length checking I already had, and the other two I added now.


> For strings, one can have deferred ("len=:",
> ts.deferred) and explicit size (len=n, len=3 etc.).

The deferred checking was still missing for the string length, but I
have added it now.


> With those changes, I am fine with the patch.

Thanks. I will commit the attached version after another regtest -
unless there are further complaints in the meantime.

[Your further remarks I will ignore for now. For some of them, one
should maybe open a PR to keep track.]

Cheers,
Janus




> PS: Some remarks, triggered by your patch but only remotely related. Though,
> I wouldn't mind if you (or someone else) could work on one of those ;-)
>
> Side remark 1: Talking about generic resolution: I think we should at some
> point implement diagnostic for generic calls as well. If the interface
> matches (according to ambiguity rules), it makes sense to also print an
> error if there is a mismatch. Currently, gfortran just prints that no
> specific proc to the a generic name has been found, which requires extensive
> digging until one find the "correct" specific function and sees that, e.g.,
> the dummy procedure has an argument with intent(inout) and the one of the
> acutal
> argument has no intent specified. [PR40276.]
>
> Side remark 1a: F2008 has relaxed some ambiguity rules; at some point we
> have to implement them. (PR 45521)
>
> Side remark 2: I think we should split the type-rank mismatch diagnostic and
> print the gfc_typename() and, respectively, rank in the error message; one
> probably should pass on the errmsg as one also needs to check the kind and
> not only the type.
>
> (I recently run into the issues of side remark 1 and 2: First the
> no-specific-proc-found error; then, calling the specific function, the
> type/rank error; it took us a while to see that the type was okay about not
> the rank.)
>
> Side remark 3: I probably introduced a bug with my recent
> assumed-type/assumed-rank patches: It regards assumed-type/assumed-rank
> compatible to any other type/rank. That makes sense for checking dummy
> arguments - but not to check the interface of dummy procedures. For
> assumed-rank, an as->type check might catch it (untested), but I fear that
> type(*) is mishandled.
>
> Side remark 4: The following program ICEs in check_assumed_size_reference's
> 1386      if ((e->ref->u.ar.end[e->ref->u.ar.as->rank - 1] == NULL)
> as e->ref == NULL.
>
> implicit none
> contains
> function g()
>   integer :: g(*)
> end function g
>
> subroutine test()
>   procedure(g), pointer :: x
>   x => g
> end subroutine test
> end
>
>
>> 2012-08-05  Janus Weil  <ja...@gcc.gnu.org>
>>
>>         PR fortran/35831
>>         * interface.c (check_result_characteristics): New function, which
>> checks
>>         the characteristics of function results.
>>         (gfc_compare_interfaces,gfc_check_typebound_override): Call it.
>>
>> 2012-08-05  Janus Weil  <ja...@gcc.gnu.org>
>>
>>         PR fortran/35831
>>         * gfortran.dg/dummy_procedure_5.f90: Modified.
>>         * gfortran.dg/dummy_procedure_8.f90: New.
>>         * gfortran.dg/interface_26.f90: Modified.
>>         * gfortran.dg/proc_ptr_11.f90: Modified.
>>         * gfortran.dg/proc_ptr_15.f90: Modified.
>>         * gfortran.dg/proc_ptr_result_5.f90: Modified.
>>         * gfortran.dg/typebound_override_1.f90: Modified.
>>         * gfortran.dg/typebound_proc_6.f03: Modified.

Attachment: pr35831_v2.diff
Description: Binary data

Attachment: dummy_procedure_8.f90
Description: Binary data

Reply via email to