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

[Daniel Kraft]

Hi FX,

here's a new version of my patch with (some of) your comments addressed;
I'll comment what I changed below.

I apologize for some of my prior comments, I did misunderstand some of
your points.

BTW, is it generally ok to have as many testcases as possible, or is
there some "upper limit" on the number you think reasonable for one
special feature?  And BTW, is there a way to make dejagnu run single
tests so I can test my dejagnu-testcase without having to do a full
check-gfortran?

Thank you for all your help so far,
Daniel

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

I did add new testcases for range-check conversion of big integers,
nested array constructors with typespec and both valid and invalid array
constructor for derived types.  The whole testsuite succeeds for me.

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

For this I didn't do anything by now, if it is ok I suggest we look at 
this separatly?

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

Renamed the variable to typespec_chararray_ctor; is this what you meant?

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

Now using gfc_conv_expr_type and adding the pre- and post blocks.

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

Did adapt comments and code.

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

Did this, too (if I used gfc_extract_int correctly).

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

Removed the braces (and the use of mpz_get_si here, too).  For the 
error, I believe this is what you talked about with Tobias, right (i.e., 
no need as reducing length is valid)?

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

-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pr27997.txt
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20080426/57b054ec/attachment.txt>