Re: [patch, libgfortran] PR114304 - [13/14 Regression] libgfortran I/O – bogus "Semicolon not allowed as separator with DECIMAL='point'"

[Jerry D <jvdelisle2@gmail.com>]

On 4/4/24 2:31 AM, Tobias Burnus wrote:
> Hi Jerry,
> 
> Jerry D wrote:
>> The attached log entry and patch (git show) fixes this issue by adding 
>> logic to handle spaces in eat_separators. One or more spaces by 
>> themselves are a valid separator. So in this case we look at the 
>> character following the spaces to see if it is a comma or semicolon.
>>
>> If so, I change it to the valid separator for the given decimal mode, 
>> point or comma. This allows the comma or semicolon to be interpreted 
>> as a null read on the next effective item in the formatted read.
>>
>> I chose a permissive approach here that allows reads to proceed when the
>> input line is mal-formed with an incorrect separator as long as there 
>> is at least one space in front of it.
> 
> First: Consider also adding 'PR fortran/105473' to the commit log
> as the PRs are closely related, albeit this PR is different-
> 
> The patch looks mostly like I would expect, except for decimal='point' 
> and a ';' which is *not* preceded by a space.
> 
> Thanks for working on it.
> 
> Regarding the 'except' case:
> 
> * * *
> 
> If I try your patch with the testcase of at comment 19,
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114304#c19
> → https://gcc.gnu.org/bugzilla/attachment.cgi?id=57695 ,
> 
> I do note that with 'decimal=point', a tailing semicolon is silently
> accepted – even if not proceeded by a space.

I did that on purpose out of leniency and fear of breaking something.  I 
will add that in and do some testing.

> 
> I think such code is invalid – and you could consider to reject it.
> Otherwise, the handling all seems to be in line with the Fortran spec.
> 
> i.e. for the following string, I had *expected an error*:

I will see what it does. I agree in principle.

> 
>   point, isreal =  F , testinput = ";"n=          42  ios=           0
>   point, isreal =  F , testinput = "5;"n=           5  ios=           0
>   point, isreal =  T , testinput = "8;"r=   8.00000000      ios= 0
>   point, isreal =  T , testinput = "3.3;"r=   3.29999995      ios= 0
>   point, isreal =  T , testinput = "3,3;"r=   3.00000000      ios= 0
> 
> while I think the following is OK (i.e. no error is what I expect) due 
> to the the space before the ';'.

Agree and this is what I was attempting.

> 
>   point, isreal =  F , testinput = "7 ;"n=           7  ios= 0
>   point, isreal =  T , testinput = "9 ;"r=   9.00000000      ios= 0
>   point, isreal =  T , testinput = "4.4 ;"r=   4.40000010      ios=0
>   point, isreal =  T , testinput = "9 ;"r=   9.00000000      ios= 0
>   point, isreal =  T , testinput = "4,4 ;"r=   4.00000000      ios= 0
> 
> * * *
> 
> Looking at the other compilers, ifort, ifx and Flang do issue an error 
> here. Likewise, g95 seems to yield an error in this case (see below).
> 
> I do note that the Lapack testcase that triggered this PR did have such 
> a code - but it was then changed because g95 did not like it:
> 
> https://github.com/Reference-LAPACK/lapack/commit/64e8a7500d817869e5fcde35afd39af8bc7a8086
> 

I am glad they fixed that, it triggered a whole lot of cycles here.

> In terms of gfortran: until recently did accept it (all versions, 
> including 13+14); it then rejected it due to the change in PR105473 (GCC 
> 14/mainline, backported to 13)– but I now think it rightly did so. With 
> the current patch, it is accepted again.
> 
> * * *
> 
> I have attached the modified testcase linked above; consider adding it 
> as well. - Changes to the one of the attachment:
> - I added a few additional (albeit boring) tests
> - I added an expected output + error diagnostic.
> 
> The testcase assumes an error for ';' as separator (with 'point'), 
> unless there is a space before it.
> 
> [If we want to not diagnose this as vendor extension, we really need to 
> add a comment to that testcase besides changing valid = .false. to .true.]

I have gone back and forth on this issue many times trying to find the 
middle ground. The standard is fairly clear. To me it is on the user to 
not use malformed input so allowing with the intervening space we are 
simply ignoring the trailing ',' or ';' and calling the spaces 
sufficient as a valid separator.

Regardless I now have your modified test case passing. Much appreciated.

Thanks for the reviews.  I will resubmit when I can, hopefully today.

Cheers,

Jerry