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....@gmail.com>
Paul Thomas<pa...@gcc.gnu.org>
Tobias Burnus<bur...@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....@gmail.com>
Damian Rouson<dam...@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.
