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

[Tobias Burnus <burnus at net-b dot 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.