Re: PING: [patch, fortran] PR 27997: Implement F2003-style array constructor with typespec

[FX <fxcoudert at gmail dot com>]

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/