-
Notifications
You must be signed in to change notification settings - Fork 140
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow fast_float to parse strings accepted by the Fortran internal read function. #219
Conversation
It is reasonable. I don't expect that this PR is likely to cause problems. |
It passes our tests, so I think we will make it part of the next release. I will let people review and react before I merge. |
That's great! Thank you very much! |
(p != pend) && | ||
((UC('e') == *p) || (UC('E') == *p))) | ||
|| | ||
((fmt & chars_format::fortran) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line would match chars_format::general as well, allowing 'd' and 'D' even in non-fortran mode.
eg. general = 0b101, fortran = 0b10101, general & fortran = 0b101 which evaluates to true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm… that’s not good. We need to fix this mistake.
@allenbarnett5 Can you react to @mayawarrier comment? To test whether we allow the fortran format, we need something like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that there is a chance that the test for fmt & chars_format::fortran
be true even when fmt
is not chars_format::fortran
which could lead to confusion and possibly to bugs where our generic parser could accept fortran strings as valid.
@allenbarnett5 Please resync with our main branch. |
@allenbarnett5 I will merge locally and issue a few fixes. |
Hi: now I'm confused :-) What should I do?
…On Fri, Sep 15, 2023 at 9:21 AM Daniel Lemire ***@***.***> wrote:
@allenbarnett5 <https://github.com/allenbarnett5> I will merge locally
and issue a few fixes.
—
Reply to this email directly, view it on GitHub
<#219 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABOLFXLKM5OZYE4ESXP2THTX2RI5XANCNFSM6AAAAAA3G7O3BE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@allenbarnett5 Nothing: I merged your PR after making changes. |
Regarding Maya's comment, I guess if there is more than one bit set in fmt,
the test needs to be something like:
(fmt&FASTFLOAT_FORTRANFMT) == FASTFLOAT_FORTRAN
or even
fmt == FASTFLOAT_FORTRAN
so that all the bits are taken into account. Sorry for the confusion.
…On Fri, Sep 15, 2023 at 9:58 AM Daniel Lemire ***@***.***> wrote:
@allenbarnett5 <https://github.com/allenbarnett5> Nothing: I merged your
PR after making changes.
—
Reply to this email directly, view it on GitHub
<#219 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABOLFXO5VGUGTWQYC3RKT7LX2RNIDANCNFSM6AAAAAA3G7O3BE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Hi: I'm working on a program which reads an input file which is ordinarily read by a Fortran program. Numbers in the input are converted to binary with the Fortran internal read function. For example,
CHARACTER(LEN=25) :: CHARS
REAL(KIND=8) :: NUMBER
READ ( CHARS, '(e25.0)' ) NUMBER
The Fortran internal read accepts a larger variety of formats than std::from_chars. In particular, the 'E' in the exponent is optional (!); it can also be replaced with a 'D'. A leading '+' is also accepted. So, "+1+1" is 10.
I made a few changes to fast_float to support this case. I realize it's somewhat out of scope for a C++ function, but it is useful to me. fast_float is much better than my attempt.
I added a test, but no other CMake infrastructure. Since it does add a condition in parse_number_string, I suppose it slows down the parsing. So, one might want to make it a CMake option and add some conditional #ifdef's. Also, it should #define FASTFLOAT_ALLOWS_LEADING_PLUS. I can add this as an option, but I didn't want to mess with the build infrastructure too much (without approval :-)
This is my first ever Github pull request.
Thanks,
Allen