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

[Daniel Kraft]

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

Thanks for the second review, here's another updated patch; I believe
this could be the final one if you are ok with it.

Updated ChangeLog and patch attached, tested on GNU/Linux i686 without
failures.  (I'm not sure what I have to do exactly for bootstrapping,
but I did a full successful rebuild with the patch applied, too.)

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

Filed a new PR for this, will work on it there.

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

Did remove the comment (this was the only change to the patch) after I
gave it some new thoughts; honestly, I wasn't able to figure out what
exactly was wrong without typespec_chararray_ctor there, but I believe
it is "correct" to have it here as this simply means we're only visiting
the new branch if there was a typespec given, which is probably what we
want.

When I added this comment I somehow had the impression this check should
be superfluous as ss->expr->ts.cl->length was enough, but is seems this
could be set by some other ways as a typespec and adding the explicit
typespec-check is ok and the right thing.

Ok with this?  (BTW, this test-case mentioned is for PR31219)

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


-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pr27997.changelog
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20080503/50be8436/attachment.ksh>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: pr27997.txt
URL: <http://gcc.gnu.org/pipermail/fortran/attachments/20080503/50be8436/attachment.txt>