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

Fix JSValue casting #307

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

metarutaiga
Copy link

@metarutaiga metarutaiga commented May 23, 2024

I found MSVC Compatibility #16
But it modified too many codes

And I tried MSVC 2022 to compile it today.
It just needs to be modified the JSValue casting because it's not standard C code.

If I want to build it with MSVC 2022,
I need to add -std:c11 -experimental:c11atomics option and metarutaiga@10f9f7c only.

Yeah, I dont like MSVC too.

@saghul
Copy link
Contributor

saghul commented May 23, 2024

It just needs to be modified the JSValue casting because it's not standard C code.

Can you clarify that is non-standard about it?

@metarutaiga
Copy link
Author

metarutaiga commented May 23, 2024

It just needs to be modified the JSValue casting because it's not standard C code.

Can you clarify that is non-standard about it?

You can add -Wpedantic into Makefile.

 ifdef CONFIG_CLANG
   HOST_CC=clang
   CC=$(CROSS_PREFIX)clang
   CFLAGS+=-g -Wall -MMD -MF $(OBJDIR)/$(@F).d
+  CFLAGS += -Wpedantic
   CFLAGS += -Wextra
   CFLAGS += -Wno-sign-compare
   CFLAGS += -Wno-missing-field-initializers

Result:

./quickjs.h:675:12: warning: C99 forbids casting nonscalar type 'JSValue' (aka 'struct JSValue') to the same type [-Wpedantic]
    return (JSValue)v;
           ^        ~
./quickjs.h:684:12: warning: C99 forbids casting nonscalar type 'JSValue' (aka 'struct JSValue') to the same type [-Wpedantic]
    return (JSValue)v;
           ^        ~

@saghul
Copy link
Contributor

saghul commented May 23, 2024

Ah, right. Does MSVC choke on it? On QuickJS-ng we somehow didn't run into this...

@metarutaiga
Copy link
Author

Ah, right. Does MSVC choke on it? On QuickJS-ng we somehow didn't run into this...

Maybe MSVC is use C89 default.

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