Re: [Fortran, patch] PR 52158 - Regression on character function with gfortran 4.7

[Alessandro Fanfarillo <fanfarillo dot gcc at gmail dot com>]

Hello all,

in attachment there's the new candidate patch, revisited by Tobias,
for PR fortran/45170, PR fortran/52158 and PR fortran/49430
(unexpectedly).

Please, check the Changelog, I don't know whether the descriptions are ok.

The patch is bootstrapped and tested on x86_64-unknown-linux-gnu.

gcc version 4.8.0 20120512 (experimental).


Have a nice weekend.

Alessandro.


2012/5/7 Tobias Burnus <burnus@net-b.de>:
> Hello,
>
>
> On 05/06/2012 09:37 PM, Alessandro Fanfarillo wrote:
>>
>> The patch is bootstrapped and tested on x86_64-unknown-linux-gnu - gcc
>> version 4.8.0 20120506 (experimental)
>
>
>> 2012-05-06 ?Alessandro Fanfarillo<fanfarillo.gcc@gmail.com>
>> ? ? ? ? ? ? Paul Thomas<pault@gcc.gnu.org>
>> ? ? ? ? ? ? Tobias Burnus<burnus@net-b.de>
>>
>> ? ? ? ? PR fortran/52158
>> ? ? ? ? * resolve.c (resolve_fl_derived0): Add a new condition in the if
>> ? ? ? ? statement of the deferred-length character component error block.
>> ? ? ? ? * trans-expr (gfc_conv_procedure_call): Add new checks in the if
>> ? ? ? ? statement on component's attributes (regarding PR 45170).
>
>
> Typo: "trans-expr" -> "trans-expr.c".
>
> Well, either the second PR is related to the commit - then one should have
> ? ?PR fortran/52158
> ? ?PR fortran/45170
> - or it is only that vaguely related that one should only mention it in the
> patch submittal email.
>
> As it fixes an ICE mentioned in PR 45170, I would add it. (I think one
> should close that PR, create PRs for the remaining issues and possibly a
> meta bug for tracking. That PR is really a mess.)
>
> The description "Add new checks in the if statement on component's
> attributes" is true but it makes the implicit assumption that the reader
> knows that the commit is about "ts.deferred". I had written something like
> the following:
>
> ? ? ? ?* resolve.c (resolve_fl_derived0): Allow TBPs with deferred-length
> results.
> ? ? ? ?* trans-expr (gfc_conv_procedure_call): Handle TBP with
> deferred-length results.
>
>
> I am sure one can write a nicer ChangeLog text, but at least it points to
> TBP (type-bound procedures) and to functions which have
> deferred-length-character result variable. (Actually, procedure-pointer
> components are also affected.)
>
>
>> 2012-05-06 ?Alessandro Fanfarillo<fanfarillo.gcc@gmail.com>
>> ? ? ? ? ? ? ? ? ? Damian Rouson<damian@rouson.net>
>>
>> ? ? ? ?PR fortran/45170
>
>
> Typically, the bug reporters are only acknowledged via the PR itself (and
> sometimes via a comment in the test case). That should be sufficient.
>
> Additionally, you should quote the same PRs as in the actual patch
> (fortran/ChangeLog). The test case tests that PR 52158 is fixed - and it
> tests that the bug reported in comment 19 of PR 45170 is solved.
>
>> - ? ? ? ? if (ts.deferred&& ?(sym->attr.allocatable || sym->attr.pointer))
>> + ? ? ? ? if (ts.deferred&& ?((!comp&& ?(sym->attr.allocatable
>> + ? ? ? ? ? ? ?|| sym->attr.pointer)) || (comp&& ?(comp->attr.allocatable
>> + ? ? ? ? ? ? ?|| comp->attr.pointer))))
>
>
> The spacing is wrong: You have a 14 " " before the "||" but you should have
> 1 tab followed by 6 spaces. (I don't think that this is a problem of the
> email client as the "if" line still have a tab.)
>
> Additionally, the line breaks have been wrongly placed; it should be:
>
> + ? ? ? ? if (ts.deferred
> +&& ?((!comp&& ?(sym->attr.allocatable || sym->attr.pointer))
> + ? ? ? ? ? ? ? ? || (comp&& ?(comp->attr.allocatable ||
> comp->attr.pointer))))
>
>
> That way one sees more easily which belongs together. The "&&" is below
> "ts"; the "||" is one to the right of the "(" to which this part of the
> expression belongs.
>
>
>> --- gcc-4.8/gcc/testsuite/gfortran.dg/deferred_type_param_3.f90 1970-01-01
>> 01:00:00.000000000 +0100
>> +++ gcc-4.8-patched/gcc/testsuite/gfortran.dg/deferred_type_param_3.f90
>> 2012-05-06
>> 19:26:29.498829273 +0200
>> @@ -0,0 +1,21 @@
>> +! { dg-do compile }
>> +!
>> +! PR fortran/45170
>
>
> As above: You should also list PR fortran/52158.
>
> As I realized when looking at the ChangeLog: Proc pointers are also
> affected. I think one could append the following code to the test case,
> which gives the same error without the patch. (Untested with the patch.)
>
> module test
> ?implicit none
> ?type t
> ? ?procedure(deferred_len), pointer, nopass :: ppt
> ?end type t
> contains
> ?function deferred_len()
> ? ?character(len=:), allocatable :: deferred_len
> ? ?deferred_len = 'abc'
> ?end function deferred_len
> ?subroutine doIt()
> ? ?type(t) :: x
> ? ?x%ppt => deferred_len
> ? ?if ("abc" /= x%ppt()) call abort ()
> ?end subroutine doIt
> end module test
>
> use test
> call doIt ()
> end
>
>
> Otherwise, the patch looks OK.
>
> Thanks for the contribution - and congratulation to your first GCC patch.
>
> Tobias
>
> PS: For bean counters: There is a GCC copyright assignment entry for
> Alessandro since 2012-04-18.

Attachment: deferred_last.diff
Description: Binary data