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

feat: Add support for compiling with Microsoft Visual Studio C++ (MSVC) #246

Merged
merged 9 commits into from Jan 16, 2024

Conversation

CGQAQ
Copy link
Contributor

@CGQAQ CGQAQ commented Jan 15, 2024

No description provided.

@saghul
Copy link
Contributor

saghul commented Jan 15, 2024

Thanks for getting this started! Any chance you can add a MSVC target to the CI so it's properly tested? Thank you!

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Jan 15, 2024

Sure, I'll add that later

@saghul
Copy link
Contributor

saghul commented Jan 15, 2024

Also note you can't assume MSVC always if _MSC_VER is defined. It could be Clang-CL. See the failing tests.

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Jan 15, 2024

Also note you can't assume MSVC always if _MSC_VER is defined. It could be Clang-CL. See the failing tests.

Yeah! I already noticed that, should check __clang__ is not defined as well

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Jan 16, 2024

Done, @saghul PTAL

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Jan 16, 2024

Seems like we have some issues on netbsd CI enviroment
image

@saghul
Copy link
Contributor

saghul commented Jan 16, 2024

I re-ran it and it's all good now.

Copy link
Contributor

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Great work! Left some comments, please take a look!

.gitignore Show resolved Hide resolved
cutils.h Show resolved Hide resolved
cutils.h Outdated Show resolved Hide resolved
cutils.h Outdated Show resolved Hide resolved
cutils.h Outdated Show resolved Hide resolved
cutils.h Show resolved Hide resolved
CGQAQ and others added 2 commits January 16, 2024 16:40
Co-authored-by: Saúl Ibarra Corretgé <s@saghul.net>
@CGQAQ
Copy link
Contributor Author

CGQAQ commented Jan 16, 2024

done

@CGQAQ
Copy link
Contributor Author

CGQAQ commented Jan 16, 2024

Sorry, I clicked commit suggestion, but did not pay attention the change was wrong

image

@saghul saghul merged commit 48e4c63 into quickjs-ng:master Jan 16, 2024
33 checks passed
@saghul
Copy link
Contributor

saghul commented Jan 16, 2024

Thank you!

@CGQAQ CGQAQ deleted the msvc branch January 16, 2024 11:56
@ggcrunchy
Copy link

I built this yesterday and am hoping to put it to use today.

Ran into a few things along the way. (Visual Studio 2019, C11 language, and 32-bit build with XP toolset)


This also wanted an INF: https://github.com/quickjs-ng/quickjs/blob/6868fb9e2516fde4a7a3fcef113a6bb1e5ecc957/quickjs.c#L46698


In Release mode, the floor and ceil lines complain about not being constant.

I found this post and with its #pragma solution resolved it.


Owing to the 32-bit build it didn't like the _BitScanReverse64 in cutils.h, so I #ifdef'd it to use this.

Wasn't sure how you'd want to actually handle that (or the previous point) if I did a PR.


Also when adding the newly-built library to a project it balked at the JS_NewCFunctionMagic function in quickjs.h.

I'm including it from a C++ file—default library standard, i.e. C++14—so I figured it was due to the designated initializer (there's even a suggestive comment lower down, above some macros: /* Note: c++ does not like nested designators */). Simply commenting the function out made it compile fine.

@saghul
Copy link
Contributor

saghul commented Jan 18, 2024

A PR would be most welcome!

@ggcrunchy
Copy link

Okay, cool. After I putz around with it a bit I'll get on that. 😄

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.

None yet

3 participants