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

MSVC++ 12.0 float hex string parse workaround #362

Closed
wants to merge 1 commit into from

Conversation

Cellule
Copy link
Contributor

@Cellule Cellule commented Mar 21, 2017

Allow parse_double_hex and parse_float_hex to parse strings without a power.

In MSVC++ 12.0 (and previous versions ?) the float parsing doesn't support hex strings.
Not totally sure if it's a bug or just not supported, but this change is a workaround it for older MSVC compilers.

I tried to minimize changes to not affect modern compilers as strtof and strtod give the right result.
I used parse_float_hex and parse_double_hex to parse hex strings and fallback on normal implementation if not an hex string.

Allow parse_double_hex and parse_float_hex to parse strings without a power.
@Cellule Cellule requested a review from binji March 21, 2017 01:10
@binji
Copy link
Member

binji commented Mar 21, 2017

Is this affecting some new code? I checked to see where strtof and strtod are used, and it's currently only in literal.cc, where it's only used when it's known that there are no hex floats.

#if _MSC_VER <= 1800
float strtof(const char *nptr, char **endptr) {
const char* end = nptr + strlen(nptr);
// review:: should we check for leading whitespaces ?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, probably better to match the behavior of strtof and slip whitespace.

float strtof(const char *nptr, char **endptr) {
const char* end = nptr + strlen(nptr);
// review:: should we check for leading whitespaces ?
if (string_starts_with(nptr, end, "0x")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can start with a leading '+' or '-' too.

@Cellule
Copy link
Contributor Author

Cellule commented Mar 21, 2017

Is this affecting some new code? I checked to see where strtof and strtod are used, and it's currently only in literal.cc, where it's only used when it's known that there are no hex floats.

It is not affecting new code. Only older MSVC compiler have bad float parsing implementation.
Right now (f32.const 0xf32) is a valid float format and we'll parse it in literal.cc parse_float with LiteralType::Int

I noticed that some recent changes prevent building in VS 2013. Notably the use of constexpr and noexcept keywords.
I wonder if we want to keep supporting VS2013, initially I was doing this work because we were supporting it in Chakra, but we are talking about dropping support soon chakra-core/ChakraCore#2549

@binji
Copy link
Member

binji commented Mar 22, 2017

It is not affecting new code. Only older MSVC compiler have bad float parsing implementation.
Right now (f32.const 0xf32) is a valid float format and we'll parse it in literal.cc parse_float with LiteralType::Int

That seems to be the correct behavior (checked against spec interpreter). Makes sense, I think, since we don't want to make the lexer context-sensitive.

I noticed that some recent changes prevent building in VS 2013. Notably the use of constexpr and noexcept keywords.
I wonder if we want to keep supporting VS2013, initially I was doing this work because we were supporting it in Chakra, but we are talking about dropping support soon chakra-core/ChakraCore#2549

I'm not opposed to supporting VS2013 for now (though I know some others are eager to drop it). If we decide to keep it, we'll have to add an appveyor build so it doesn't regress.

@Cellule
Copy link
Contributor Author

Cellule commented Mar 23, 2017

I will close this PR for now.
I found out that some other compiler also have similar issue. I have to investigate what to do about it. I will probably just make a patch for windows

@Cellule Cellule closed this Mar 23, 2017
@Cellule Cellule mentioned this pull request Mar 23, 2017
@Cellule Cellule deleted the parse_float branch March 27, 2017 00:10
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.

2 participants