| Summary: | Fortran 2003: Support type-spec for array constructor | ||
|---|---|---|---|
| Product: | gcc | Reporter: | tobias.burnus |
| Component: | fortran | Assignee: | Not yet assigned to anyone <unassigned> |
| Status: | RESOLVED FIXED | ||
| Severity: | enhancement | CC: | d, fxcoudert, gcc-bugs, P.Schaffnit |
| Priority: | P3 | ||
| Version: | unknown | ||
| Target Milestone: | --- | ||
| Host: | Target: | ||
| Build: | Known to work: | ||
| Known to fail: | Last reconfirmed: | 2006-09-28 08:02:35 | |
| Bug Depends on: | |||
| Bug Blocks: | 20585 | ||
| Attachments: |
Patch to add this feature
Partly fixed patch Patch padding for constant character lengths Implements dynamic string length Handles bounds checking and fixes regressions. |
||
Confirmed, and marked as blocking the F2003 meta-PR. Created attachment 14512 [details]
Patch to add this feature
This patch does the job. I'll wait until next stage 1 to submit it.
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. 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 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.
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? (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. (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.) 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 (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? (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.) (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? 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? (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) 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?
(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 ;-) I see, thanks! I thought it would be the longest length (i.e., clipped by the array definition assigned to). > 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). 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 ;)
> 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? 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) ;)
> 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. (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 ;) (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...) 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 FIXED on the trunk (4.4.0) |
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 ---------------------