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

Work around msvc definition of NAN #702

Merged
merged 2 commits into from
Nov 18, 2024
Merged

Conversation

bnoordhuis
Copy link
Contributor

NAN is reportedly no longer a compile-time expression in some versions of MSVC. Work around that by minting a NaN from a uint64.

Fixes: #693

@saghul
Copy link
Contributor

saghul commented Nov 18, 2024

LoL, I took a stab at it here: #701

@@ -41116,7 +41116,7 @@ static const JSCFunctionListEntry js_number_funcs[] = {
JS_CFUNC_DEF("isSafeInteger", 1, js_number_isSafeInteger ),
JS_PROP_DOUBLE_DEF("MAX_VALUE", 1.7976931348623157e+308, 0 ),
JS_PROP_DOUBLE_DEF("MIN_VALUE", 5e-324, 0 ),
JS_PROP_DOUBLE_DEF("NaN", NAN, 0 ),
JS_PROP_U2D_DEF("NaN", 0x7FF8ull<<48, 0 ), // workaround for msvc
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to define JS_FOLAT64_NAN like so?

Copy link
Contributor Author

@bnoordhuis bnoordhuis Nov 18, 2024

Choose a reason for hiding this comment

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

Already exists and defined as follows :)

#define JS_FLOAT64_NAN NAN

edit: I suppose you're asking to change JS_FLOAT64_NAN? Won't work, you can't write it as a compile-time expression.

I initially named it JS_COMPILE_TIME_NAN but then I thought "rule of three" and there are only two instances now.

@saghul
Copy link
Contributor

saghul commented Nov 18, 2024

Can you herry pick the 1st commit from #701 so we can test if this is it?

@bnoordhuis
Copy link
Contributor Author

Can you herry pick the 1st commit from #701 so we can test if this is it?

That doesn't build/pass CI though, does it?

@saghul
Copy link
Contributor

saghul commented Nov 18, 2024

Can you herry pick the 1st commit from #701 so we can test if this is it?

That doesn't build/pass CI though, does it?

The first commit is just the CI, which fails on master, if you add it here we'd know if your change fixes it :-)

@saghul
Copy link
Contributor

saghul commented Nov 18, 2024

If this works you also want to add a "Fixes" for #622

@saghul saghul mentioned this pull request Nov 18, 2024
NAN is reportedly no longer a compile-time expression in some versions
of MSVC. Work around that by minting a NaN from a uint64.

Fixes: quickjs-ng#622
Fixes: quickjs-ng#693
@bnoordhuis bnoordhuis merged commit f93dd58 into quickjs-ng:master Nov 18, 2024
11 checks passed
@bnoordhuis bnoordhuis deleted the fix693 branch November 18, 2024 09:49
@bnoordhuis
Copy link
Contributor Author

Oh, for posterity: another approach would have been to define them as e.g. 0.0/0.0 (and I guess that's still an option for JS_FLOAT64_NAN) but the benefit of bitcasting u64 to double is that it lets us control the exact NaN bit pattern.

@saghul
Copy link
Contributor

saghul commented Nov 18, 2024

Oh, for posterity: another approach would have been to define them as e.g. 0.0/0.0

I read in some other GH issue that that approach was no bueno for some reason with MSVC 🤷

but the benefit of bitcasting u64 to double is that it lets us control the exact NaN bit pattern.

TIL, makes sense!

@bnoordhuis
Copy link
Contributor Author

Let's test it: #704 :)

@saghul
Copy link
Contributor

saghul commented Nov 18, 2024

Speaking of JS_FLOAT64_NAN is there a compelling reason to use that instead of NAN directly? I see more usages of the latter across quickjs.c

@bnoordhuis
Copy link
Contributor Author

Not that I can see. I considered removing it but one thing at a time.

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.

warnings from msvc 2022
2 participants