| Message ID | CAHqFgjV3YqCi67LKh-ttt1KNYjtd-O462K=HdrUATNK02wM6YA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
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 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
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
--- 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);
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