Hi Mikael, hi all, 2014-02-22 16:38 GMT+01:00 Mikael Morin : > Le 19/02/2014 16:51, Janus Weil a écrit : >> The patch was not applying cleanly any more, so here is a re-diffed >> version for current trunk. It works nicely on the included test case >> as well as the one provided by Walter Spector in comment 12 of the PR. >> >> Since, also in the current state, "character(:)" works only in a >> subset of all cases, I think it cannot hurt to add more cases that >> work for 4.9 (even if still not all possible cases work). >> >> Please let me know what you think ... > > Review: > >> PR fortran/51976 >> * gfortran.h : Add deferred_parameter attribute. > Add the name of the struct before ':' > like "(struct symbol_attribute)" or maybe just "(symbol_attribute)" > >> * trans.c (gfc_deferred_strlen): New function. >> * trans.h : Prototype for the new function. > This is really nitpicking but "(gfc_deferred_strlen)" should be in front > of trans.h as well. Done. > Now regarding the patch itself, I don't know character handling very > well, but it seems to me that the patch makes the (wrong) assumption > that characters are of default kind, so that string length is the same > as memory size. > Namely: > >> Index: gcc/fortran/trans-array.c >> =================================================================== >> --- gcc/fortran/trans-array.c (revision 207896) >> +++ gcc/fortran/trans-array.c (working copy) >> @@ -7365,7 +7365,7 @@ get_full_array_size (stmtblock_t *block, tree decl >> >> static tree >> duplicate_allocatable (tree dest, tree src, tree type, int rank, >> - bool no_malloc) >> + bool no_malloc, tree strlen) >> { >> tree tmp; >> tree size; >> @@ -7386,7 +7386,11 @@ duplicate_allocatable (tree dest, tree src, tree t >> null_data = gfc_finish_block (&block); >> >> gfc_init_block (&block); >> - size = TYPE_SIZE_UNIT (TREE_TYPE (type)); >> + if (strlen != NULL_TREE) >> + size = strlen; >> + else >> + size = TYPE_SIZE_UNIT (TREE_TYPE (type)); >> + > here... Fixed (but actually in the code which calls duplicate_allocatable). >> size = fold_build2_loc (input_location, MULT_EXPR, gfc_array_index_type, >> nelems, tmp); >> if (!no_malloc) >> Index: gcc/fortran/trans-expr.c >> =================================================================== >> --- gcc/fortran/trans-expr.c (revision 207896) >> +++ gcc/fortran/trans-expr.c (working copy) >> @@ -6043,9 +6051,40 @@ gfc_trans_subcomponent_assign (tree dest, gfc_comp >> gfc_add_expr_to_block (&block, tmp); >> } >> } >> - else >> + else if (gfc_deferred_strlen (cm, &tmp)) >> { >> - /* Scalar component. */ >> + tree strlen; >> + strlen = tmp; >> + gcc_assert (strlen); >> + strlen = fold_build3_loc (input_location, COMPONENT_REF, >> + TREE_TYPE (strlen), >> + TREE_OPERAND (dest, 0), >> + strlen, NULL_TREE); >> + >> + if (expr->expr_type == EXPR_NULL) >> + { >> + tmp = build_int_cst (TREE_TYPE (cm->backend_decl), 0); >> + gfc_add_modify (&block, dest, tmp); >> + tmp = build_int_cst (TREE_TYPE (strlen), 0); >> + gfc_add_modify (&block, strlen, tmp); >> + } >> + else >> + { >> + gfc_init_se (&se, NULL); >> + gfc_conv_expr (&se, expr); >> + tmp = build_call_expr_loc (input_location, >> + builtin_decl_explicit (BUILT_IN_MALLOC), >> + 1, se.string_length); > here, Also fixed (again by using size_of_string_in_bytes). >> + gfc_add_modify (&block, dest, >> + fold_convert (TREE_TYPE (dest), tmp)); >> + gfc_add_modify (&block, strlen, se.string_length); >> + tmp = gfc_build_memcpy_call (dest, se.expr, se.string_length); >> + gfc_add_expr_to_block (&block, tmp); >> + } >> + } >> + else if (!cm->attr.deferred_parameter) >> + { >> + /* Scalar component (excluding deferred parameters). */ >> gfc_init_se (&se, NULL); >> gfc_init_se (&lse, NULL); >> >> Index: gcc/fortran/trans-stmt.c >> =================================================================== >> --- gcc/fortran/trans-stmt.c (revision 207896) >> +++ gcc/fortran/trans-stmt.c (working copy) >> @@ -5028,6 +5028,11 @@ gfc_trans_allocate (gfc_code * code) >> if (tmp && TREE_CODE (tmp) == VAR_DECL) >> gfc_add_modify (&se.pre, tmp, fold_convert (TREE_TYPE (tmp), >> memsz)); >> + else if (al->expr->ts.type == BT_CHARACTER >> + && al->expr->ts.deferred && se.string_length) >> + gfc_add_modify (&se.pre, se.string_length, >> + fold_convert (TREE_TYPE (se.string_length), >> + memsz)); >> > and here. There may be other places that I have missed. Actually I don't see a problem here. Also no further modifications were necessary to get a KIND=4 example to work. >> /* Convert to size in bytes, using the character KIND. */ >> if (unlimited_char) > > As the patch seems to provide a wanted feature, and as the new code > seems to be properly guarded, I'm not against it after the above has > been checked and fixed if necessary. Attached is a new version of the patch which fixes the above-mentioned problems and should make the non-default-kind cases work. I have added a KIND=4 version of the original test case, which seems to work properly. The patch was regtested on x86_64-unknown-linux-gnu. Ok for trunk? Btw, there was quite some user feedback along the lines of "we want this feature", but there was not a lot of actual testing. So, if you are interested in this feature, please try the patch and report back here with things like "I tested it and it works great on my 100kLOC code" or "I tested it and found the following problem ..." Cheers, Janus 2014-03-01 Paul Thomas gcc.gnu.org> Janus Weil PR fortran/51976 * gfortran.h (symbol_attribute): Add deferred_parameter attribute. * primary.c (build_actual_constructor): It is not an error if a missing component has the deferred_parameter attribute; equally, if one is given a value, it is an error. * resolve.c (resolve_fl_derived0): Remove error for deferred character length components. Add the hidden string length field to the structure. Give it the deferred_parameter attribute. * trans-array.c (duplicate_allocatable): Add a strlen field which is used as the element size if it is non-null. (gfc_duplicate_allocatable, gfc_copy_allocatable_data): Pass a NULL to the new argument in duplicate_allocatable. (structure_alloc_comps): Set the hidden string length as appropriate. Use it in calls to duplicate_allocatable. (gfc_alloc_allocatable_for_assignment): When a deferred length backend declaration is variable, use that; otherwise use the string length from the expression evaluation. * trans-expr.c (gfc_conv_component_ref): If this is a deferred character length component, the string length should have the value of the hidden string length field. (gfc_trans_subcomponent_assign): Set the hidden string length field for deferred character length components. Allocate the necessary memory for the string. (alloc_scalar_allocatable_for_assignment): Same change as in gfc_alloc_allocatable_for_assignment above. * trans-stmt.c (gfc_trans_allocate): Likewise. * trans-intrinsic (size_of_string_in_bytes): Make non-static. * trans-types.c (gfc_get_derived_type): Set the tree type for a deferred character length component. * trans.c (gfc_deferred_strlen): New function. * trans.h (size_of_string_in_bytes,gfc_deferred_strlen): New prototypes. 2014-03-01 Paul Thomas gcc.gnu.org> Janus Weil PR fortran/51976 * gfortran.dg/deferred_type_component_1.f90 : New test. * gfortran.dg/deferred_type_component_2.f90 : New test.