This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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<>
             Paul Thomas<>
             Tobias Burnus<>

         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<>
                   Damian Rouson<>

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

Otherwise, the patch looks OK.

Thanks for the contribution - and congratulation to your first GCC patch.


PS: For bean counters: There is a GCC copyright assignment entry for Alessandro since 2012-04-18.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]