Patchwork [Fortran] PR 52158 - Regression on character function with gfortran 4.7

login
register
mail settings
Submitter Alessandro Fanfarillo
Date May 6, 2012, 7:37 p.m.
Message ID <CAHqFgjV3YqCi67LKh-ttt1KNYjtd-O462K=HdrUATNK02wM6YA@mail.gmail.com>
Download mbox | patch
Permalink /patch/157190/
State New
Headers show

Comments

Alessandro Fanfarillo - May 6, 2012, 7:37 p.m.
Hello,

my name is Alessandro, I'm a newbie of GCC and helped by Tobias Burnus
and Paul Thomas I'll try to add support for final subroutines.

The patch is bootstrapped and tested on x86_64-unknown-linux-gnu - gcc
version 4.8.0 20120506 (experimental)

Best regards.


------------------------------------------------gcc/fortran/ChangeLog------------------------------------------------

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).

------------------------------------------------gcc/testsuite/ChangeLog------------------------------------------------

2012-05-06  Alessandro Fanfarillo  <fanfarillo.gcc@gmail.com>
                  Damian Rouson  <damian@rouson.net>

        PR fortran/45170
        * gfortran.dg/deferred_type_param_3.f90: New.

------------------------------------------------Patch.diff------------------------------------------------

diff -urN gcc-4.8/gcc/testsuite/gfortran.dg/deferred_type_param_3.f90
gcc-4.8-patched/gcc/testsuite/gfortran.dg/deferred_type_param_3.f90
--- 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
+!
+! Contributed by Damian Rouson
+
+module speaker_class
+  type speaker
+  contains
+    procedure :: speak
+  end type
+contains
+  function speak(this)
+    class(speaker) ,intent(in) :: this
+    character(:) ,allocatable :: speak
+  end function
+  subroutine say_something(somebody)
+    class(speaker) :: somebody
+    print *,somebody%speak()
+  end subroutine
+end module
Tobias Burnus - May 7, 2012, 8:23 a.m.
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.
Janus Weil - May 7, 2012, 9 a.m.
Hi Alessandro,

> my name is Alessandro, I'm a newbie of GCC and helped by Tobias Burnus
> and Paul Thomas I'll try to add support for final subroutines.

since Tobias already reviewed your patch, I just want to say: Welcome
to the team :)

We're certainly looking forward to your contributions! Having final
subroutines in gfortran will be great progress.

Btw, one thing that you may want to look into on your way to FINAL is
proper deallocation of polymorphic variables(cf.
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46321). This is one thing
which is still missing in our OOP implementation, and it's serious in
the sense that almost any sensible OOP program produces memory leaks
with gfortran right now. It is not exactly a prerequisite to FINAL,
but at least similar in spirit. Maybe both features can even be
implemented with similar techniques. I actually had some plans to
implement it myself, but currently I'm not able to spend large amounts
of time on gfortran (unfortunately).

In any case, I wish you all the best for your efforts ...

Cheers,
Janus
Alessandro Fanfarillo - May 12, 2012, 11:04 a.m.
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.
Tobias Burnus - May 12, 2012, 12:35 p.m.
Hello Alessandro,

Alessandro Fanfarillo wrote:
> 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.

Thanks for the patch. According to the source code, the diff was sent as 
"Content-Type: application/octet-stream", which in principle makes 
reviewing more difficult. However, in my Thunderbird, it showed up as 
inline text - thus I am fine.

I think the ChangeLog is okay - as is the whole patch.

I wouldn't mind if someone else (Paul? Janus?) could also glance at the 
patch; however, if there are no comments, I intent to commit the patch soon.

Tobias
Tobias Burnus - May 13, 2012, 10:54 a.m.
Tobias Burnus wrote:
> I think the ChangeLog is okay - as is the whole patch.
>
> I wouldn't mind if someone else (Paul? Janus?) could also glance at 
> the patch; however, if there are no comments, I intent to commit the 
> patch soon.

Committed as Rev. 187436. Thanks for the patch and congratulation to 
your first GCC contribution!

Tobias
Alessandro Fanfarillo - May 15, 2012, 10:14 a.m.
2012/5/13 Tobias Burnus <burnus@net-b.de>:

>
> Committed as Rev. 187436. Thanks for the patch and congratulation to your
> first GCC contribution!


Thank you for your priceless support!


Alessandro

Patch

--- gcc-4.8/gcc/fortran/resolve.c	2012-05-06 19:29:21.794825508 +0200
+++ gcc-4.8-patched/gcc/fortran/resolve.c	2012-05-06 19:24:40.770831649 +0200
@@ -11666,7 +11666,7 @@ 
   for ( ; c != NULL; c = c->next)
     {
       /* See PRs 51550, 47545, 48654, 49050, 51075 - and 45170.  */
-      if (c->ts.type == BT_CHARACTER && c->ts.deferred)
+      if (c->ts.type == BT_CHARACTER && c->ts.deferred && !c->attr.function)
 	{
 	  gfc_error ("Deferred-length character component '%s' at %L is not "
 		     "yet supported", c->name, &c->loc);
diff -urN gcc-4.8/gcc/fortran/trans-expr.c
gcc-4.8-patched/gcc/fortran/trans-expr.c
--- gcc-4.8/gcc/fortran/trans-expr.c	2012-05-06 19:29:21.878825505 +0200
+++ gcc-4.8-patched/gcc/fortran/trans-expr.c	2012-05-06 19:25:53.134830069 +0200
@@ -4175,7 +4175,9 @@ 
 	     we take the character length of the first argument for the result.
 	     For dummies, we have to look through the formal argument list for
 	     this function and use the character length found there.*/
-	  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))))
 	    cl.backend_decl = gfc_create_var (gfc_charlen_type_node, "slen");
 	  else if (!sym->attr.dummy)
 	    cl.backend_decl = VEC_index (tree, stringargs, 0);