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

[FX]

Hi Daniel,

Here is my review of your revised patch. First, a small formal issue:  
you need to provide updated ChangeLog entries with an updated patch.  
Second, you need to indicate what testing has been done on the new  
patch and where (again, something along the lines of "bootstrapped  
and regtested on x86_64-linux, with both -m32 and -m64). [1]   Third,  
until you have your copyright assignment confirmed, no commit  
possible, so I propose you send us a final, updated version of your  
patch after we work on the last comments below and ChangeLog entries,  
for reference until we can commit it.


[1] For those who might ask, the command-line to run a regtest with  
both -m32 and -m64 on a 64-bit multilibbed host such as x86_64-linux is:
make check-gfortran RUNTESTFLAGS="--target_board=unix'{-m32,-m64}'" ${*}



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

OK. But please give it some thought if you have a bit of time.

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

OK.

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

Yep, I was mistaken to suggest an error.

> +      /* get_array_ctor_strlen walks the elements of the  
> constructor, if a
> +	 typespec was given, we already know the string length and want  
> the one
> +	 specified there.  */
> +      /* XXX:  Adding typespec_chararray_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?  */
> +      if (typespec_chararray_ctor && ss->expr->ts.cl->length
> +	  && ss->expr->ts.cl->length->expr_type != EXPR_CONSTANT)


I'd prefer we find out and don't leave such a comment (and question)  
in the code: "adding" will not mean anything to the reader when the  
rest of the code has changed, and now is the best time to solve this  
issue. What is the backtrace of the ICE if you leave  
typespec_chararray_ctor out?

FX

-- 
François-Xavier Coudert
http://www.homepages.ucl.ac.uk/~uccafco/