PING: [patch, fortran] PR 27997: Implement F2003-style array constructor with typespec
FX
fxcoudert@gmail.com
Fri Apr 25 13:33:00 GMT 2008
Hi Daniel,
Ante-scriptum 1: Any news from you copyright assignment?
Ante-scriptum 2: I'm reviewing this one a plane, so I don't get
access to web and bugzilla... I might say thus report an issue that
is already know :)
Please find my review below. It's globally a well-written patch,
following GNU coding standards and has quite a testcases, which is
good and we sometime overlook a bit (writing elaborate testcases
covering all cases one can think of is a tiresome exercise). This is
not yet OK to commit (although it's fairly close), I suspect you'll
want to update it and respond to some of my comments below.
> 2008-04-07 Daniel Kraft <d@domob.eu>
>
> PR fortran/27997
> * gfortran.h: Added field "length_from_typespec" to
> gfc_charlength needed for the patch.
> * array.c: Added parsing of typespec to
> gfc_match_array_constructor and extended
> gfc_resolve_character_array_constructor to correctly
> pad strings when a constant character length was given
> by a typespec.
> * trans-array.c: Added code to gfc_trans_array_constructor
> to support explicit but dynamic character lengths.
The correct format includes the names of each function touched:
YYYY-MM-DD Joe Hacker <joe@hacker.name>
PR ######
* file1.h (function1): This is what I changed in function1.
(function2): This is what I changed in function2.
(function3): Again.
* file2.c (function1, function2): Made same changes in both
functions.
> 2008-04-07 Daniel Kraft <d@domob.eu>
>
> PR fortran/27997
> * gfortran.dg/array_constructor_type_1.f03: New test
> * gfortran.dg/array_constructor_type_2.f03: New test
> * gfortran.dg/array_constructor_type_3.f03: New test
> * gfortran.dg/array_constructor_type_4.f03: New test
> * gfortran.dg/array_constructor_type_5.f03: New test
> * gfortran.dg/array_constructor_type_6.f03: New test
> * gfortran.dg/array_constructor_type_7.f03: New test
> * gfortran.dg/array_constructor_type_8.f03: New test
> * gfortran.dg/array_constructor_type_9.f: New test
> * gfortran.dg/array_constructor_type_10.f03: New test
> * gfortran.dg/array_constructor_type_11.f03: New test
> * gfortran.dg/array_constructor_type_12.f03: New test
> * gfortran.dg/array_constructor_type_13.f90: New test
I am wondering: could you add some testcases with failing conversion
to the constructor type. I see four cases I'd like covered: a too
long string, like that:
write *, [ character(len=1) :: "I am too long", "me too" ]
and a value that doesn't fit in the constructor type, like that:
write *, [ integer(4) :: huge(0_8) ]
Also, I think we should also include a testcase with derived types:
type foo
integer i
real x
end type foo
type(foo), parameter :: x = foo(42, 42.)
print *, [type(foo) :: x, foo(0,1) ]
end
(this one just shows that it works, but is not dejagnu-ified). Also,
a rejected conversion of derived types:
print *, [type(foo) :: x, foo(0,1), bar(.true.) ]
1
Error: Can't convert TYPE(bar) to TYPE(foo) at (1)
And constructors withing constructors, like this:
print *, [ integer(kind=8) :: 4, [ integer(kind=4) :: 42, 12 ] ]
print *, [ integer(kind=8) :: [ integer(kind=4) :: 42, 12 ] ]
print *, [ integer(kind=8) :: [ integer(kind=4) :: ] ]
print *, [ character(len=8) :: "foo", [ character(len=4) :: "bar",
"gee" ] ]
print *, [ character(len=8) :: "foo", [ character(len=4) :: ] ]
print *, [ character(len=8) :: [ character(len=4) :: ] ]
end
OK, let's dig in:
> Index: gcc/fortran/trans-array.c
> ===================================================================
> --- gcc/fortran/trans-array.c (revision 133867)
> +++ gcc/fortran/trans-array.c (working copy)
> @@ -960,9 +960,10 @@
> }
>
>
> -/* Assign an element of an array constructor. */
> +/* Variables needed for bounds-checking. */
> static bool first_len;
> static tree first_len_val;
> +static bool typespec_ctor;
>
> static void
> gfc_trans_array_ctor_element (stmtblock_t * pblock, tree desc,
> @@ -999,7 +1000,7 @@
> se->string_length,
> se->expr);
> }
> - if (flag_bounds_check)
> + if (flag_bounds_check && !typespec_ctor)
> {
> if (first_len)
> {
I've tried to trigger that code and could not see it work. I cannot
see a runtime error, neither with
call test ("this is long")
contains
subroutine test(s)
character(len=*) :: s
print *, [ character(len=3) :: s, "foo" ]
end subroutine test
end
nor with
call test ("this is long")
contains
subroutine test(s)
character(len=*) :: s
print *, [ s, "foo" ]
end subroutine test
end
As this was not working previously, I'm not asking you to fix it in
this patch :) But, being now our most proficient expert in array
constructors, you might have an opinion as to why that doesn't
work... Otherwise, please file a PR so it's fixed later.
> @@ -1681,7 +1682,11 @@
> tree type;
> bool dynamic;
>
> - if (flag_bounds_check && ss->expr->ts.type == BT_CHARACTER)
> + /* Do bounds-checking here and in gfc_trans_array_ctor_element
> only if no
> + typespec was given for the array constructor. */
> + typespec_ctor = (ss->expr->ts.cl && ss->expr->ts.cl-
> >length_from_typespec);
This means you only ever set typespec_ctor for character arrays. This
is fine with me, because that's the information we need, but I
suggest giving the variable a name that indicates this fact (lest
someone uses it for other type).
> + /* XXX: Adding typespec_ctor here was merely a hack to make
> + gfortran.dg/array_constructor_17.f90 work (ICE otherwise). Is this
> + ok anyway or should I look for another solution? */
I can't tell, because there is no gfortran.dg/
array_constructor_17.f90 testcase in your patch :) It depends on why
we had an ICE, but it looks OK as it is.
> + /* XXX: This works for my tests, but is this the correct way to
> + transform the gfc_expr into a tree? */
Ah... well, it rather depends... you've got the right function, but
I'd suggest here using the variant with a type, gfc_conv_expr_type():
> + gfc_init_se (&length_se, NULL);
> + gfc_conv_expr (&length_se, ss->expr->ts.cl->length);
That second line would be :
gfc_conv_expr_type (&length_se, ss->expr->ts.cl-
>length, gfc_charlen_type_node);
> + ss->string_length = length_se.expr;
That one is fine. But, you have created gfc_se, and it might have pre
and post blocks (i.e. things that were added before and after the
loop, like evaluation of some loop invariant quantities). This is
done by:
gfc_add_block_to_block (&loop->pre, &length_se.pre);
gfc_add_block_to_block (&loop->post, &length_se.post);
which adds these pre and post blocks to the pre and post blocks of
the global loop (passed as a parameter to the function).
> /* Update the character length of the array constructor. */
> expr->ts.cl->length = gfc_int_expr (max_length);
> - /* Update the element constructors. */
> - for (p = expr->value.constructor; p; p = p->next)
> - if (p->expr->expr_type == EXPR_CONSTANT)
> - gfc_set_constant_character_len (max_length, p->expr, true);
> + generated_length = true;
> + /* Real update follows below. */
Hum, the comments should be rewritten, removing the "historical"
ones, like:
/* Mark
> + if (expr->ts.cl->length->ts.type == BT_INTEGER)
> + {
> + if (expr->ts.cl->length->expr_type == EXPR_CONSTANT)
> + {
> + /* Got a constant character length, pad according to this. */
> + max_length = mpz_get_si (expr->ts.cl->length->value.integer);
> + }
> + }
> + }
I imagine this is written that way for historical reasons. The proper
style would be:
/* We've got a character length specified. It should be an
integer,
otherwise an error is signalled elsewhere. */
gcc_assert (expr->ts.cl->length);
/* If we've got a constant character length, pad according to
this. */
if (expr->ts.cl->length->ts.type == BT_INTEGER
&& expr->ts.cl->length->expr_type == EXPR_CONSTANT)
max_length = mpz_get_si (expr->ts.cl->length->value.integer);
(indentation of this code might be weird, as I'm writing this
directly in the mailer).
As for the use of mpz_get_si() directly on the value, it is done too
often in currently existing code but is not a good coding practice:
the expr->ts.cl->length->value.integer might not fit in a host
integer. Please use gfc_extract_int() instead. (It is m project, at
some point, to audit all uses of mpz_get_si in the front-end, but I
never got around to it; we should also have a function
gfc_extract_hwint that returns a HOST_WIDE_INT instead of a
gfc_extract_int, for that matter).
> + if (generated_length || ! cl
> + || (cl->expr_type == EXPR_CONSTANT
> + && cl->ts.type == BT_INTEGER
> + && mpz_get_si (cl->value.integer) < max_length))
> + {
> + gfc_set_constant_character_len (max_length, p->expr, true);
> + }
Unneeded braces. It looks like we could easily emit an error for
strings of constant length that are too long, couldn't we?
>
Thanks for this very fine patch. I look forward to reading your
comments.
FX
--
François-Xavier Coudert
http://www.homepages.ucl.ac.uk/~uccafco/
More information about the Fortran
mailing list