Bug 27997 - Fortran 2003: Support type-spec for array constructor
Fortran 2003: Support type-spec for array constructor
Status: RESOLVED FIXED
Product: gcc
Classification: Unclassified
Component: fortran
unknown
: P3 enhancement
: ---
Assigned To: Not yet assigned to anyone
:
Depends on:
Blocks: 20585
  Show dependency treegraph
 
Reported: 2006-06-12 12:50 UTC by tobias.burnus
Modified: 2008-05-16 19:54 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2006-09-28 08:02:35


Attachments
Patch to add this feature (3.65 KB, patch)
2007-11-08 18:59 UTC, Francois-Xavier Coudert
Details | Diff
Partly fixed patch (6.95 KB, patch)
2008-03-24 20:01 UTC, Daniel Kraft
Details | Diff
Patch padding for constant character lengths (10.33 KB, patch)
2008-03-25 19:53 UTC, Daniel Kraft
Details | Diff
Implements dynamic string length (14.22 KB, patch)
2008-04-04 20:27 UTC, Daniel Kraft
Details | Diff
Handles bounds checking and fixes regressions. (17.56 KB, patch)
2008-04-06 15:28 UTC, Daniel Kraft
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description tobias.burnus 2006-06-12 12:50:45 UTC
See Fortran 2003 standard, section 4.8.
Example (F2003, "Note 4.70"):
  (/ CHARACTER(LEN=7) :: ’Takata’, ’Tanaka’, ’Hayashi’ /)

Currently, this gives the error:
  a =  (/ character(len=7) :: 'Takata', 'Tanaka', 'Hayashi' /)
                         1
Error: Syntax error in array constructor at (1)

---------------------
program test
  character(15) :: a(3)
  a =  (/ character(len=7) :: 'Takata', 'Tanaka', 'Hayashi' /)
  print '(a)',a
end program test
---------------------
Comment 1 Francois-Xavier Coudert 2006-09-28 08:02:34 UTC
Confirmed, and marked as blocking the F2003 meta-PR.
Comment 2 Francois-Xavier Coudert 2007-11-08 18:59:08 UTC
Created attachment 14512 [details]
Patch to add this feature

This patch does the job. I'll wait until next stage 1 to submit it.
Comment 3 Tobias Burnus 2007-12-14 16:24:28 UTC
As noted by Tobias Schlüter, the following valid program is rejected:

INTEGER = 1
PRINT *, (/ INTEGER /)
END

One solution is to check for the "::" and if it does not exists, set "seen_ts = 0" and "gfc_current_locus = where".
One also has to move the gfc_notify_std down (e.g. after "done:") and check then whether seen_ts is set. I think that's what I would do.
Comment 4 Tobias Burnus 2007-12-14 17:19:55 UTC
By the way, your patch does not work, unless I messed up locally. For the example of comment 0 I get "Hayash" and not "Hayashi":

  static character(kind=1)[1:6] * A.1[3]= {&"Takata"[1]{ ...

Test case:

! { dg-do run }
!
! PR fortran/27997
!
! Support array constructors with typespec.
!
program test
  implicit none
  character(15) :: a(3)
  a =  (/ character(len=7) :: 'Takata', 'Tanaka', 'Hayashi' /)
  if ( len([ character(len=7) :: ]) /= 7) call abort()
  if ( size([ integer :: ]) /= 0) call abort()
  if(     a(1) /= 'Takata'  .or. a(1)(7:7)   /= achar(32) &
                            .or. a(1)(15:15) /= achar(32) &
     .or. a(2) /= 'Tanaka'  .or. a(2)(7:7)   /= achar(32) &
                            .or. a(2)(15:15) /= achar(32) &
     .or. a(3) /= 'Hayashi' .or. a(3)(8:8)   /= achar(32))&
                            .or. a(1)(15:15) /= achar(32))&
   call abort()
end program test
Comment 5 Daniel Kraft 2008-03-24 20:01:37 UTC
Created attachment 15370 [details]
Partly fixed patch

This patch fixes the problem described above with respect to correct detection of seen_ts and adds some tests (hope this is ok); the string conversion problem is still failing.
Comment 6 Daniel Kraft 2008-03-25 12:44:08 UTC
After looking at the assembly dump, the problem with the strings seems to be that the literals are stored packed to each other in the data section and the array constructor simply lists the pointers to them and sets the length value to 7; thus, the shorter strings get the following character (that happens to be the first one of the next string) appended to them.

Could we resolve this problem by keeping track for each string literal in which content it will accessed and padding them with spaces to the longest used length when dumping to the data section?  Possibly when gfc_convert_expr is called on a string literal?

BTW, is this a problem that affects other things also, for instance, user defined records?
Comment 7 Francois-Xavier Coudert 2008-03-25 12:51:07 UTC
(In reply to comment #6)

This string length change (and padding) needs to happen way earlier than that. We should make it happen during the matching, when we convert the constructor objects into the array constructor type.
Comment 8 Francois-Xavier Coudert 2008-03-25 12:58:11 UTC
(In reply to comment #6)
> Possibly when gfc_convert_expr is called on a string literal?

I'm not sure but I think you'd see the same issue on a character variable that is not a constant. gfc_convert_expr() doesn't handle conversion of characters into different strings lengths, and it probably shouldn't be extended to do that (it's a special case that is only used in type-spec array constructors). So we probably need to add some code to handle that instead of calling gfc_convert_expr() when we have character variables. (Maybe a function to do just that already exists in expr.c.)
Comment 9 Tobias Burnus 2008-03-25 16:04:45 UTC
Another test case, causes an ICE with another compiler, which supports the first test case; I have not tested it with the current patch.

program test
  character(15) :: a(3)
  character(10), volatile :: b(3)
  b(1) = 'Takata'
  b(2) = 'Tanaka'
  b(3) = 'Hayashi'
  a =  (/ character(len=7) :: trim(b(1)), trim(b(2)), trim(b(3)) /)
  print '(a)',a
  a =  (/ character(len=2) :: trim(b(1)), trim(b(2)), trim(b(3)) /)
  print '(a)',a
  a =  (/ character(len=8) :: trim(b(1)), trim(b(2)), trim(b(3)) /)
  print '(a)',a
end program test
Comment 10 Daniel Kraft 2008-03-25 16:40:54 UTC
(In reply to comment #7)
> (In reply to comment #6)
> 
> This string length change (and padding) needs to happen way earlier than that.
> We should make it happen during the matching, when we convert the constructor
> objects into the array constructor type.

Hm, what about adding a case to gfc_resolve_character_array_constructor handling arrays *with* given length by padding all elements that are shorter?  Would this work?
Comment 11 Francois-Xavier Coudert 2008-03-25 16:54:31 UTC
(In reply to comment #10)
> Hm, what about adding a case to gfc_resolve_character_array_constructor
> handling arrays *with* given length by padding all elements that are shorter? 
> Would this work?

Sounds like the right place. Care should be taken because lengths are not required to be constants, like in the following:

  call foo(8, "short")
  call foo(2, "lenghty")
contains
  subroutine foo(n,s)
    character(len=*) s
    integer n
    print *, [ character(len=n) :: 'test', s ]
  end subroutine
end

(I haven't yet found a compiler that does compile this and run it fine.)
Comment 12 Daniel Kraft 2008-03-25 17:10:29 UTC
(In reply to comment #11)
> Sounds like the right place. Care should be taken because lengths are not
> required to be constants, like in the following:
> 
>   call foo(8, "short")
>   call foo(2, "lenghty")
> contains
>   subroutine foo(n,s)
>     character(len=*) s
>     integer n
>     print *, [ character(len=n) :: 'test', s ]
>   end subroutine
> end

Well, I'm not sure if the patch I've got in mind would handle this...  I'm thinking of padding string literals shorter than the array's length at compile time when this length is constant.

For non-constant lengths I suppose a runtime call would be needed to pad correctly?  Is this important or should I first make constant lengths work?
Comment 13 Daniel Kraft 2008-03-25 19:53:45 UTC
Created attachment 15374 [details]
Patch padding for constant character lengths

This patch implements padding if the character length of the array is constant (it was much easier than I thought, and much less complicated than the solution I tried before--just know where...).  It handles the original failure and also the test from comment #9 (if I understood correctly what the output should be), but it fails for the one in comment #11 with dynamic length.

I did also add those two cases to the test suite, with the latter failing.  This patch also provokes a failure of char_component_initializer_1.f90 of pr31487.  Any hints why?
Comment 14 Tobias Burnus 2008-03-28 10:32:04 UTC
(In reply to comment #13)
> Created an attachment (id=15374) [edit]
> Patch padding for constant character lengths

I assume that you have not bootstrapped GCC/gfortran since with that patch it fails when compiling libgfortran/intrinsics/selected_real_kind.f90.

Reduced test case:

  type :: real_info
    integer :: kind
  end type real_info
  type (real_info) :: real_infos(1) = (/ real_info (4) /)
end


  type (real_info) :: real_infos(1) = (/ real_info (4) /)
                                            1
Error: Syntax error in array constructor at (1)
Comment 15 Daniel Kraft 2008-04-01 08:12:26 UTC
The bootstrap problem is fixed, but with dynamic lengths I'm still struggling ;)
However, the following program without typespec in the array constructor also fails (according to my judging what it should do) both with 4.3 and my (patched) SVN version; I don't have access to an unpatched one:

PROGRAM test
  CALL foo (8, "short")
CONTAINS
  SUBROUTINE foo (n, s)
    INTEGER :: n
    CHARACTER(len=*) :: s
    CHARACTER(len=n) :: arr(2)
    arr = (/ 'test', s /)
    WRITE (*,*) arr(1)
    WRITE (*,*) arr(2)
    WRITE (*,*) s
    WRITE (*,*)
  END SUBROUTINE foo
END PROGRAM test

Is this anouther bug or do I misinterpret something?
Comment 16 Tobias Burnus 2008-04-01 10:17:28 UTC
(In reply to comment #15)
> However, the following program without typespec in the array constructor also
> fails (according to my judging what it should do)

Different compilers give different output for this one, but this is no problem as the program is invalid. "gfortran -fbounds-check" shows the problem:

Fortran runtime error: Different CHARACTER lengths (4/5) in array constructor

That is the reason why (/ typespec :: element-list /)  is needed. How long should be each element for  (/ "a", "bb", "ccc" /) ? One, two, three or ... characters? One could argue that is should match the longest, but this is an arbitrary choice and makes the run-time array construction slower. Actually, with the Intel compiler, 's, "test"' and '"test", s' produce different results ;-)
Comment 17 Daniel Kraft 2008-04-01 12:53:57 UTC
I see, thanks!  I thought it would be the longest length (i.e., clipped by the array definition assigned to).
Comment 18 Tobias Burnus 2008-04-01 14:18:01 UTC
> I see, thanks!  I thought it would be the longest length (i.e., clipped by the
> array definition assigned to).

For completeness: See "4.7 Construction of array values" in the Fortran 2003 standard (http://gcc.gnu.org/wiki/GFortranStandards). There one finds:

"C494 (R466) If type-spec is omitted, each ac-value expression in the array-constructor shall have the same type and kind type parameters."

This does not apply yet as we have everywhere default-kind character variables. But it continues a few lines later as follows (note the "same length type parameter"):

"If type-spec is omitted, each ac-value expression in the array constructor shall have the same length type parameters; in this case, the type and type parameters of the array constructor are those of the ac-value expressions.

"If type-spec appears, it specifies the type and type parameters of the array constructor. Each ac-value expression in the array-constructor shall be compatible with intrinsic assignment to a variable of this type and type parameters.[...]"


In general, it is quite helpful to look at the specification in the Fortran standard, when one implements something. (Although it takes some time to get somewhat used to the standard; still I find it quite readable.) A Fortran textbook or Reid's F2003 intro (ftp://ftp.nag.co.uk/sc22wg5/N1601-N1650/N1648.pdf) are helpful, but at the end the standard is the only official and definite reference. If you need interpretation help or help searching, write me an email (or try the IRC, link see http://gcc.gnu.org/wiki/GFortran). It if gets too difficult, one ends up asking at comp.lang.fortran or even requesting an official interpretation (cf. PR 34805).
Comment 19 Daniel Kraft 2008-04-04 20:27:09 UTC
Created attachment 15429 [details]
Implements dynamic string length

Finally, I managed to get dynamic string length working; however, this was more an experimentation thing for some parts.  For instance, the generation of the tree-expression in gfc_trans_array_constructor was pure guessing and getting lucky, so I fear it is not quite done the right way; however, so far it works and I hope it is nearly correct now.

This patch makes all my tests for constructor with typespec work, but it introduces two regression failures I'll still have to look at.  And finally, I fear the style is yet to become perfect ;)
Comment 20 Tobias Burnus 2008-04-04 22:43:35 UTC
> Implements dynamic string length
Great!

But you also need the handle -fbounds-check. Compile the test case of comment 11 with -fbounds-check. The result is:

Fortran runtime error: Different CHARACTER lengths (4/5) in array constructor

Without type spec the error is correct, however, with the type spec the elements are allowed to have different lengths.

For the test cases: How about adding one test case for -std=f95, which rejects this Fortran 2003 feature?
Comment 21 Daniel Kraft 2008-04-06 15:28:57 UTC
Created attachment 15435 [details]
Handles bounds checking and fixes regressions.

This patch handles the bounds checking correctly, adds a test case if -std=f95 rejects array constructors with typespec, and fixes all regression failures, i.e., this makes the testsuite pass cleanly including all my own new test cases.

However, I'm not quite sure about some things I did and if this is really the right way (although it seems to work), marked them mostly with XXX:.  I'd be glad if someone could have a look at the patch and those places especially to see if all is ok or give comments how it should be done.

And last but not least I'm not sure if I hit the right code style, I mostly doubt I did correctly insert the tab characters where they belong (as I'm used to indent with spaces only) ;)
Comment 22 Tobias Burnus 2008-04-06 16:22:19 UTC
> Created an attachment (id=15435) [edit]
> Handles bounds checking and fixes regressions.

Great.

> However, I'm not quite sure about some things I did [...]

I think your patch is in a good enough shape to post it to fortran@gcc.gnu.org and ask there for comments; this ensures that all gfortraners read it (and not only three Fortraners) and it makes discussing easier than a in a bugreport.

I think one could also submit it for review, which means that you need to write a changelog and should CC the email to gcc-patches@.  (The reason is that then also developers of the middle end can glance at it and give comments.)

+/* XXX: Ok as global or does bounds-checking need to be reentrant for things
+   like nested constructors?  */

I think it is OK for now, but we need to fix it later. I found a test case which crashes gfortran (ICE, internal compiler error) for nested character constructors - even without typespec or bounds checks. See PR 35846. I think this should be fixed together with the rest.
Comment 23 Daniel Kraft 2008-04-08 13:11:16 UTC
(In reply to comment #22)
> > However, I'm not quite sure about some things I did [...]
> 
> I think your patch is in a good enough shape to post it to fortran@gcc.gnu.org
> and ask there for comments; this ensures that all gfortraners read it (and not
> only three Fortraners) and it makes discussing easier than a in a bugreport.

Ok,  sent it in.

> I think it is OK for now, but we need to fix it later. I found a test case
> which crashes gfortran (ICE, internal compiler error) for nested character
> constructors - even without typespec or bounds checks. See PR 35846. I think
> this should be fixed together with the rest.

I thought about possible solutions when introducing my global variable, namely either to pass them on the argument list down the stack (which is probably what I would do myself), or to abuse the function stack by saving the old value when setting and resetting it on return; while this seems a bit hackish to me, it means probably less changes...  In any case, PR 35846 could be a nice other bug for me once this one is done ;)
Comment 24 Daniel Kraft 2008-04-11 20:26:29 UTC
(In reply to comment #23)
> (In reply to comment #22)
> > I think your patch is in a good enough shape to post it to fortran@gcc.gnu.org
> > and ask there for comments; this ensures that all gfortraners read it (and not
> > only three Fortraners) and it makes discussing easier than a in a bugreport.
> 
> Ok,  sent it in.

Any news from the patch?  Or is it usual to take some time to get responses? Hope it didn't get lost (or the responses in the gcc-patches volume on my machine...)
Comment 25 Tobias Burnus 2008-05-16 19:50:55 UTC
Subject: Bug 27997

Author: burnus
Date: Fri May 16 19:50:04 2008
New Revision: 135439

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=135439
Log:
2008-04-16  Daniel Kraft  <d@domob.eu>

        PR fortran/27997
        * gfortran.h:  Added field "length_from_typespec" to gfc_charlength.
        * aray.c (gfc_match_array_constructor):  Added code to parse
        * typespec.
        (check_element_type, check_constructor_type, gfc_check_constructor_type):
        Extended to support explicit typespec on constructor.
        (gfc_resolve_character_array_constructor):  Pad strings correctly for
        explicit, constant character length.
        * trans-array.c:  New static global variable
        * "typespec_chararray_ctor"
        (gfc_trans_array_constructor):  New code to support explicit but dynamic
        character lengths.

2008-04-16  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
        * gfortran.dg/array_constructor_type_14.f03:  New test
        * gfortran.dg/array_constructor_type_15.f03:  New test
        * gfortran.dg/array_constructor_type_16.f03:  New test
        * gfortran.dg/array_constructor_type_17.f03:  New test
        * gfortran.dg/array_constructor_type_18.f03:  New test


Added:
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_1.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_10.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_11.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_12.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_13.f90
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_14.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_15.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_16.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_17.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_18.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_2.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_3.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_4.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_5.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_6.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_7.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_8.f03
    trunk/gcc/testsuite/gfortran.dg/array_constructor_type_9.f
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/array.c
    trunk/gcc/fortran/gfortran.h
    trunk/gcc/fortran/trans-array.c
    trunk/gcc/testsuite/ChangeLog

Comment 26 Tobias Burnus 2008-05-16 19:54:50 UTC
FIXED on the trunk (4.4.0)