Skip to content
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

Merged
merged 1 commit into from
Sep 15, 2023

Conversation

allenbarnett5
Copy link

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

@lemire
Copy link
Member

lemire commented Aug 7, 2023

It is reasonable. I don't expect that this PR is likely to cause problems.

@lemire
Copy link
Member

lemire commented Aug 7, 2023

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.

@allenbarnett5
Copy link
Author

That's great! Thank you very much!

(p != pend) &&
((UC('e') == *p) || (UC('E') == *p)))
||
((fmt & chars_format::fortran) &&
Copy link
Contributor

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.

Copy link
Member

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.

@lemire
Copy link
Member

lemire commented Sep 14, 2023

@allenbarnett5 Can you react to @mayawarrier comment?

To test whether we allow the fortran format, we need something like fmt & (1<<4) or something... (e.g., fmt == chars_format::fortran).

Copy link
Member

@lemire lemire left a 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.

@lemire
Copy link
Member

lemire commented Sep 15, 2023

@allenbarnett5 Please resync with our main branch.

@lemire
Copy link
Member

lemire commented Sep 15, 2023

@allenbarnett5 I will merge locally and issue a few fixes.

@allenbarnett5
Copy link
Author

allenbarnett5 commented Sep 15, 2023 via email

@lemire lemire merged commit 7646f81 into fastfloat:main Sep 15, 2023
37 checks passed
@lemire
Copy link
Member

lemire commented Sep 15, 2023

@allenbarnett5 Nothing: I merged your PR after making changes.

@allenbarnett5
Copy link
Author

allenbarnett5 commented Sep 15, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants