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

[Daniel Kraft]

FX wrote:
> Ante-scriptum 1: Any news from you copyright assignment?

As already written, I'm still waiting on the confirmation.

> 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.

Thanks for the review, I'll comment/correct on the items.

>> 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:

OK, I see.  BTW, should I add these lines to the corresponding 
ChangeLog-files, too, or should this be done on check-in?

Regarding the test-cases, I'll add some with respect to your discussion 
with Tobias.

>> 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.

Hm, are you sure about this?  I'm not sure myself at the moment, but I 
*think* I had to add this to get it work for -fbounds-check.  But 
anyways, then we'll leave it like this, right?

>> @@ -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).

So something like typespec_chararray_ctor?

> 
>> +      /* 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.

Ok, I'll for the moment leave this in as-is including the comment.

>> +      /* 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():

I see, so gfc_conv_expr_type is the thing one should generally use when 
the type is known?  I did find gfc_conv_expr merely by unstructured 
search in the codebase ;)

>> +      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).

I'll do it.  But my main concern was on correctly releasing anything 
that should be after the block and if it's safe to simply extract the 
.expr-member of length_se.  But if this is, I'll gladly leave it like this.

>> +      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);

I remember I tried it like this first, but this assertion failed; so I 
concluded this ==BT_INTEGER test is needed but it is simply ok to ignore 
the case when it is no integer, as this raises an error anyway.

> 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).

Cool, I even like gfc_extract_int much better than doing it directly, I 
just didn't know for know that there is this function :D

>> +        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?

I put the braces in because it looks better for me than this long 
condition followed by the statement without braces; but if it's common 
practice to *never* set braces around single statements, I'll do it like 
this in the future.

For the emitting the error, I'm not sure if I understand you correctly; 
or is this the thing you talked about with Tobias, so obsolete?

Cheers,
Daniel

-- 
Done:     Bar-Sam-Val-Wiz, Dwa-Elf-Hum-Orc, Cha-Law, Fem-Mal
Underway: Ran-Gno-Neu-Fem
To go:    Arc-Cav-Hea-Kni-Mon-Pri-Rog-Tou