| Message ID | 20150319161346.7041ea23@vepi2 |
|---|---|
| State | New |
| Headers | show |
Dear Andre, I have applied the three preliminary patches but have not yet applied the attached one for PR55901. As advertised the composite patch bootstraps and regtests on FC21,x86_64. I went through gfc_trans_allocate and cleaned up the formatting and some of the text in the comments. You did a heroic job to tidy up this function and so I thought that I should do my bit - one of the feature, previously, was that the line length often went well in excess of the gcc style guide limit of 72 and this tended to make it somewhat unreadable. I have not been rigorous about this, especially when readability would be impaired thereby, but it does look a lot better now. The composite diff is attached. Not only does the Metcalf example run correctly but also the PGI Insider linked list example. I have attached a version of this modified to function as a gfortran.dg testcase. With the attributions in there, I do not think that there are any copyright issues. The article itself has no copyright notice. I would very much like to say that this is OK for trunk but we are hard up against the end of stage 4 and so it should really wait for backporting to 5.2. Thanks for the patches Paul On 19 March 2015 at 16:13, Andre Vehreschild <vehre@gmx.de> wrote: > Hi all, > > please find attached the parts missing to stop valgrind's complaining about the > use of uninitialized memory. The issue was, that when constructing a temporary > class-object to call a routine with unlimited polymorphic arguments, the _len > component was never set. This is fixed by this patch now. > > Note, the patch is based on all these preliminary patches: > > https://gcc.gnu.org/ml/fortran/2015-03/msg00074.html > https://gcc.gnu.org/ml/fortran/2015-03/msg00075.html > https://gcc.gnu.org/ml/fortran/2015-03/msg00085.html > > Bootstraps and regtests ok on x86_64-linux-gnu/F20. > > Please review! > > - Andre > -- > Andre Vehreschild * Email: vehre ad gmx dot de
On 03/21/2015 07:11 AM, Paul Richard Thomas wrote: --- snip --- > I would very much like to say that this is OK for trunk but we are > hard up against the end of stage 4 and so it should really wait for > backporting to 5.2. > IMHO, since gfortran is not release critical, we should consider, in the interest of progress, committing this to trunk now. It will give much needed exposure to OOP features and allow users to exercise the code. (Subject to release manager approval) Regards, Jerry
Dear Andre, I am persuaded by the arguments of Jerry and Dominique that this is good for trunk. Please commit as early as possible in order that any regressions can be caught, if possible, before release. Thanks Paul On 21 March 2015 at 15:11, Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote: > Dear Andre, > > I have applied the three preliminary patches but have not yet applied > the attached one for PR55901. As advertised the composite patch > bootstraps and regtests on FC21,x86_64. > > I went through gfc_trans_allocate and cleaned up the formatting and > some of the text in the comments. You did a heroic job to tidy up this > function and so I thought that I should do my bit - one of the > feature, previously, was that the line length often went well in > excess of the gcc style guide limit of 72 and this tended to make it > somewhat unreadable. I have not been rigorous about this, especially > when readability would be impaired thereby, but it does look a lot > better now. The composite diff is attached. > > Not only does the Metcalf example run correctly but also the PGI > Insider linked list example. I have attached a version of this > modified to function as a gfortran.dg testcase. With the attributions > in there, I do not think that there are any copyright issues. The > article itself has no copyright notice. > > I would very much like to say that this is OK for trunk but we are > hard up against the end of stage 4 and so it should really wait for > backporting to 5.2. > > Thanks for the patches > > Paul > > On 19 March 2015 at 16:13, Andre Vehreschild <vehre@gmx.de> wrote: >> Hi all, >> >> please find attached the parts missing to stop valgrind's complaining about the >> use of uninitialized memory. The issue was, that when constructing a temporary >> class-object to call a routine with unlimited polymorphic arguments, the _len >> component was never set. This is fixed by this patch now. >> >> Note, the patch is based on all these preliminary patches: >> >> https://gcc.gnu.org/ml/fortran/2015-03/msg00074.html >> https://gcc.gnu.org/ml/fortran/2015-03/msg00075.html >> https://gcc.gnu.org/ml/fortran/2015-03/msg00085.html >> >> Bootstraps and regtests ok on x86_64-linux-gnu/F20. >> >> Please review! >> >> - Andre >> -- >> Andre Vehreschild * Email: vehre ad gmx dot de > > > > -- > Outside of a dog, a book is a man's best friend. Inside of a dog it's > too dark to read. > > Groucho Marx
Hi Paul, thanks for the reviews. Let me ask one questions before I do something wrong. You have reviewed and approved (with changes) the patches: - vtab_access_rework1_v1.patch https://gcc.gnu.org/ml/fortran/2015-03/msg00074.html - vtab_access_rework2_v1.patch https://gcc.gnu.org/ml/fortran/2015-03/msg00075.html - pr64787_v2.patch https://gcc.gnu.org/ml/fortran/2015-03/msg00085.html and - pr55901_v1.patch https://gcc.gnu.org/ml/fortran/2015-03/msg00086.html , right? I am asking so explicitly, because there are four more patches from me in the wild, that await review (not necessarily from you, Paul), namely: - pr60322_base_1.patch https://gcc.gnu.org/ml/fortran/2015-02/msg00105.html - pr60322_3.patch https://gcc.gnu.org/ml/fortran/2015-03/msg00032.html - crashfix2_v1.patch (small patch, ~100 loc)) https://gcc.gnu.org/ml/fortran/2015-03/msg00063.html and - cosm_simp.patch (tiny patch, ~20 loc) https://gcc.gnu.org/ml/fortran/2015-03/msg00088.html Please don't get me wrong on this. I just want to prevent misunderstandings here. The latter four patches are not yet approved, right? I will now apply the 4.9-trunk patch and wait for your answer before applying the above four on vtab_rework pr64787 and pr55901. Regards, Andre On Mon, 23 Mar 2015 08:33:51 +0100 Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote: > Dear Andre, > > I am persuaded by the arguments of Jerry and Dominique that this is > good for trunk. Please commit as early as possible in order that any > regressions can be caught, if possible, before release. > > Thanks > > Paul > > On 21 March 2015 at 15:11, Paul Richard Thomas > <paul.richard.thomas@gmail.com> wrote: > > Dear Andre, > > > > I have applied the three preliminary patches but have not yet applied > > the attached one for PR55901. As advertised the composite patch > > bootstraps and regtests on FC21,x86_64. > > > > I went through gfc_trans_allocate and cleaned up the formatting and > > some of the text in the comments. You did a heroic job to tidy up this > > function and so I thought that I should do my bit - one of the > > feature, previously, was that the line length often went well in > > excess of the gcc style guide limit of 72 and this tended to make it > > somewhat unreadable. I have not been rigorous about this, especially > > when readability would be impaired thereby, but it does look a lot > > better now. The composite diff is attached. > > > > Not only does the Metcalf example run correctly but also the PGI > > Insider linked list example. I have attached a version of this > > modified to function as a gfortran.dg testcase. With the attributions > > in there, I do not think that there are any copyright issues. The > > article itself has no copyright notice. > > > > I would very much like to say that this is OK for trunk but we are > > hard up against the end of stage 4 and so it should really wait for > > backporting to 5.2. > > > > Thanks for the patches > > > > Paul > > > > On 19 March 2015 at 16:13, Andre Vehreschild <vehre@gmx.de> wrote: > >> Hi all, > >> > >> please find attached the parts missing to stop valgrind's complaining > >> about the use of uninitialized memory. The issue was, that when > >> constructing a temporary class-object to call a routine with unlimited > >> polymorphic arguments, the _len component was never set. This is fixed by > >> this patch now. > >> > >> Note, the patch is based on all these preliminary patches: > >> > >> https://gcc.gnu.org/ml/fortran/2015-03/msg00074.html > >> https://gcc.gnu.org/ml/fortran/2015-03/msg00075.html > >> https://gcc.gnu.org/ml/fortran/2015-03/msg00085.html > >> > >> Bootstraps and regtests ok on x86_64-linux-gnu/F20. > >> > >> Please review! > >> > >> - Andre > >> -- > >> Andre Vehreschild * Email: vehre ad gmx dot de > > > > > > > > -- > > Outside of a dog, a book is a man's best friend. Inside of a dog it's > > too dark to read. > > > > Groucho Marx > > >
Dear Andre, Yes, that's right. The first three (vtab rework 1/2 and pr64787) are combined and reformatted in the .diff file that I sent you. Please use that and then apply the pr55901 patch. This is what I am okaying. Cheers Paul On 23 March 2015 at 10:45, Andre Vehreschild <vehre@gmx.de> wrote: > Hi Paul, > > thanks for the reviews. Let me ask one questions before I do something wrong. > You have reviewed and approved (with changes) the patches: > > - vtab_access_rework1_v1.patch > https://gcc.gnu.org/ml/fortran/2015-03/msg00074.html > - vtab_access_rework2_v1.patch > https://gcc.gnu.org/ml/fortran/2015-03/msg00075.html > - pr64787_v2.patch > https://gcc.gnu.org/ml/fortran/2015-03/msg00085.html > and > - pr55901_v1.patch > https://gcc.gnu.org/ml/fortran/2015-03/msg00086.html > , right? > > I am asking so explicitly, because there are four more patches from me in the > wild, that await review (not necessarily from you, Paul), namely: > > - pr60322_base_1.patch > https://gcc.gnu.org/ml/fortran/2015-02/msg00105.html > - pr60322_3.patch > https://gcc.gnu.org/ml/fortran/2015-03/msg00032.html > - crashfix2_v1.patch (small patch, ~100 loc)) > https://gcc.gnu.org/ml/fortran/2015-03/msg00063.html > and > - cosm_simp.patch (tiny patch, ~20 loc) > https://gcc.gnu.org/ml/fortran/2015-03/msg00088.html > > Please don't get me wrong on this. I just want to prevent misunderstandings > here. The latter four patches are not yet approved, right? > > I will now apply the 4.9-trunk patch and wait for your answer before applying > the above four on vtab_rework pr64787 and pr55901. > > Regards, > Andre > > > > On Mon, 23 Mar 2015 08:33:51 +0100 > Paul Richard Thomas <paul.richard.thomas@gmail.com> wrote: > >> Dear Andre, >> >> I am persuaded by the arguments of Jerry and Dominique that this is >> good for trunk. Please commit as early as possible in order that any >> regressions can be caught, if possible, before release. >> >> Thanks >> >> Paul >> >> On 21 March 2015 at 15:11, Paul Richard Thomas >> <paul.richard.thomas@gmail.com> wrote: >> > Dear Andre, >> > >> > I have applied the three preliminary patches but have not yet applied >> > the attached one for PR55901. As advertised the composite patch >> > bootstraps and regtests on FC21,x86_64. >> > >> > I went through gfc_trans_allocate and cleaned up the formatting and >> > some of the text in the comments. You did a heroic job to tidy up this >> > function and so I thought that I should do my bit - one of the >> > feature, previously, was that the line length often went well in >> > excess of the gcc style guide limit of 72 and this tended to make it >> > somewhat unreadable. I have not been rigorous about this, especially >> > when readability would be impaired thereby, but it does look a lot >> > better now. The composite diff is attached. >> > >> > Not only does the Metcalf example run correctly but also the PGI >> > Insider linked list example. I have attached a version of this >> > modified to function as a gfortran.dg testcase. With the attributions >> > in there, I do not think that there are any copyright issues. The >> > article itself has no copyright notice. >> > >> > I would very much like to say that this is OK for trunk but we are >> > hard up against the end of stage 4 and so it should really wait for >> > backporting to 5.2. >> > >> > Thanks for the patches >> > >> > Paul >> > >> > On 19 March 2015 at 16:13, Andre Vehreschild <vehre@gmx.de> wrote: >> >> Hi all, >> >> >> >> please find attached the parts missing to stop valgrind's complaining >> >> about the use of uninitialized memory. The issue was, that when >> >> constructing a temporary class-object to call a routine with unlimited >> >> polymorphic arguments, the _len component was never set. This is fixed by >> >> this patch now. >> >> >> >> Note, the patch is based on all these preliminary patches: >> >> >> >> https://gcc.gnu.org/ml/fortran/2015-03/msg00074.html >> >> https://gcc.gnu.org/ml/fortran/2015-03/msg00075.html >> >> https://gcc.gnu.org/ml/fortran/2015-03/msg00085.html >> >> >> >> Bootstraps and regtests ok on x86_64-linux-gnu/F20. >> >> >> >> Please review! >> >> >> >> - Andre >> >> -- >> >> Andre Vehreschild * Email: vehre ad gmx dot de >> > >> > >> > >> > -- >> > Outside of a dog, a book is a man's best friend. Inside of a dog it's >> > too dark to read. >> > >> > Groucho Marx >> >> >> > > > -- > Andre Vehreschild * Email: vehre ad gmx dot de
diff --git a/gcc/fortran/trans-expr.c b/gcc/fortran/trans-expr.c index 7d3f3be..a30c391 100644 --- a/gcc/fortran/trans-expr.c +++ b/gcc/fortran/trans-expr.c @@ -578,6 +578,34 @@ gfc_conv_derived_to_class (gfc_se *parmse, gfc_expr *e, } } + if (class_ts.u.derived->components->ts.type == BT_DERIVED + && class_ts.u.derived->components->ts.u.derived + ->attr.unlimited_polymorphic) + { + /* Take care about initializing the _len component correctly. */ + ctree = gfc_class_len_get (var); + if (UNLIMITED_POLY (e)) + { + gfc_expr *len; + gfc_se se; + + len = gfc_copy_expr (e); + gfc_add_len_component (len); + gfc_init_se (&se, NULL); + gfc_conv_expr (&se, len); + if (optional) + tmp = build3_loc (input_location, COND_EXPR, TREE_TYPE (se.expr), + cond_optional, se.expr, + fold_convert (TREE_TYPE (se.expr), + integer_zero_node)); + else + tmp = se.expr; + } + else + tmp = integer_zero_node; + gfc_add_modify (&parmse->pre, ctree, fold_convert (TREE_TYPE (ctree), + tmp)); + } /* Pass the address of the class object. */ parmse->expr = gfc_build_addr_expr (NULL_TREE, var); @@ -736,44 +764,54 @@ gfc_conv_intrinsic_to_class (gfc_se *parmse, gfc_expr *e, } } - /* When the actual arg is a char array, then set the _len component of the - unlimited polymorphic entity, too. */ - if (e->ts.type == BT_CHARACTER) + gcc_assert (class_ts.type == BT_CLASS); + if (class_ts.u.derived->components->ts.type == BT_DERIVED + && class_ts.u.derived->components->ts.u.derived + ->attr.unlimited_polymorphic) { ctree = gfc_class_len_get (var); - /* Start with parmse->string_length because this seems to be set to a - correct value more often. */ - if (parmse->string_length) - gfc_add_modify (&parmse->pre, ctree, parmse->string_length); - /* When the string_length is not yet set, then try the backend_decl of - the cl. */ - else if (e->ts.u.cl->backend_decl) - gfc_add_modify (&parmse->pre, ctree, e->ts.u.cl->backend_decl); - /* If both of the above approaches fail, then try to generate an - expression from the input, which is only feasible currently, when the - expression can be evaluated to a constant one. */ - else - { - /* Try to simplify the expression. */ - gfc_simplify_expr (e, 0); - if (e->expr_type == EXPR_CONSTANT && !e->ts.u.cl->resolved) - { - /* Amazingly all data is present to compute the length of a - constant string, but the expression is not yet there. */ - e->ts.u.cl->length = gfc_get_constant_expr (BT_INTEGER, 4, - &e->where); - mpz_set_ui (e->ts.u.cl->length->value.integer, - e->value.character.length); - gfc_conv_const_charlen (e->ts.u.cl); - e->ts.u.cl->resolved = 1; - gfc_add_modify (&parmse->pre, ctree, e->ts.u.cl->backend_decl); - } + /* When the actual arg is a char array, then set the _len component of the + unlimited polymorphic entity, too. */ + if (e->ts.type == BT_CHARACTER) + { + /* Start with parmse->string_length because this seems to be set to a + correct value more often. */ + if (parmse->string_length) + tmp = parmse->string_length; + /* When the string_length is not yet set, then try the backend_decl of + the cl. */ + else if (e->ts.u.cl->backend_decl) + tmp = e->ts.u.cl->backend_decl; + /* If both of the above approaches fail, then try to generate an + expression from the input, which is only feasible currently, when the + expression can be evaluated to a constant one. */ else { - gfc_error ("Can't compute the length of the char array at %L.", - &e->where); + /* Try to simplify the expression. */ + gfc_simplify_expr (e, 0); + if (e->expr_type == EXPR_CONSTANT && !e->ts.u.cl->resolved) + { + /* Amazingly all data is present to compute the length of a + constant string, but the expression is not yet there. */ + e->ts.u.cl->length = gfc_get_constant_expr (BT_INTEGER, 4, + &e->where); + mpz_set_ui (e->ts.u.cl->length->value.integer, + e->value.character.length); + gfc_conv_const_charlen (e->ts.u.cl); + e->ts.u.cl->resolved = 1; + tmp = e->ts.u.cl->backend_decl; + } + else + { + gfc_error ("Can't compute the length of the char array at %L.", + &e->where); + } } } + else + tmp = integer_zero_node; + + gfc_add_modify (&parmse->pre, ctree, tmp); } /* Pass the address of the class object. */ parmse->expr = gfc_build_addr_expr (NULL_TREE, var);