Skip to content

Add C++ compile test#615

Merged
bnoordhuis merged 2 commits intoquickjs-ng:masterfrom
bnoordhuis:fix608
Oct 26, 2024
Merged

Add C++ compile test#615
bnoordhuis merged 2 commits intoquickjs-ng:masterfrom
bnoordhuis:fix608

Conversation

@bnoordhuis
Copy link
Copy Markdown
Contributor

@bnoordhuis bnoordhuis commented Oct 23, 2024

Check that quickjs.h parses without error when fed to a C++ compiler.

@bnoordhuis
Copy link
Copy Markdown
Contributor Author

bnoordhuis commented Oct 23, 2024

ffs, clang-cl.exe is on the path but cl.exe isn't. I have so many choice words for Windows and msvc.

@bnoordhuis
Copy link
Copy Markdown
Contributor Author

bnoordhuis commented Oct 23, 2024

bangs head against desk

My search engine of choice tells me cl.exe is in ex. C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\bin\Hostx64\x64 but that feels too brittle to hard-code.

@saghul ideas?

@bnoordhuis
Copy link
Copy Markdown
Contributor Author

bnoordhuis commented Oct 23, 2024 

 ParserError: D:\a\_temp\f9fe22ae-45a5-4d5d-9a50-2c85a3b8440e.ps1:3
Line |
   3 |  … erprise/VC/Tools/MSVC/17.11.35327.3/bin/Hostx64/x64/cl.exe" /Zs cxxte …
     |                                                                 ~
     | You must provide a value expression following the '/' operator.

I give up. Towel -> ring.

@saghul
Copy link
Copy Markdown
Contributor

saghul commented Oct 23, 2024

I'll take a look, thanks for getting the ball rolling!

This was referenced Oct 23, 2024
Closed
Closed
@saghul
Copy link
Copy Markdown
Contributor

saghul commented Oct 24, 2024

Got it!

Run cl.exe /Zs cxxtest.cc      
Microsoft (R) C/C++ Optimizing Compiler Version 19.41.34123 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

cxxtest.cc
D:\a\quickjs\quickjs\quickjs.h(489): error C7555: use of designated initializers requires at least '/std:c++20'
D:\a\quickjs\quickjs\quickjs.h(489): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\quickjs\quickjs\quickjs.h(494): error C7555: use of designated initializers requires at least '/std:c++20'
D:\a\quickjs\quickjs\quickjs.h(494): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\quickjs\quickjs\quickjs.h(504): error C7555: use of designated initializers requires at least '/std:c++20'
D:\a\quickjs\quickjs\quickjs.h(504): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax
D:\a\quickjs\quickjs\quickjs.h(905): error C7555: use of designated initializers requires at least '/std:c++20'
cxxtest.cc(8): error C7555: use of designated initializers requires at least '/std:c++20'
cxxtest.cc(8): error C4576: a parenthesized type followed by an initializer list is a non-standard explicit type conversion syntax

@saghul
Copy link
Copy Markdown
Contributor

saghul commented Oct 24, 2024

I suppose we should specify the standard version?

@bnoordhuis
Copy link
Copy Markdown
Contributor Author

bnoordhuis commented Oct 24, 2024

The fact you need to pull in egor-tensin/vs-shell tells me it's way harder than it should be :-)

I'll add a /std: switch and make sure it gets tested with JS_NAN_BOXING on and off. Thanks!

@saghul
Copy link
Copy Markdown
Contributor

saghul commented Oct 24, 2024

I tried a couple of things but my powershell is weak so I thought...

image

@bnoordhuis @saghul
Add C++ compile test
5a089f6 
Check that quickjs.h parses without error when fed to a C++ compiler.

Co-authored-by: Saúl Ibarra Corretgé <s@saghul.net>
@bnoordhuis
Copy link
Copy Markdown
Contributor Author

bnoordhuis commented Oct 26, 2024

JS_MKVAL and friends work okay now but the JSCFunctionListEntry initializers are never going to work - not as constant expressions, and that's their intended use case, of course.

@saghul thoughts? I could drop them from cxxtest.cc and call it a day. For many users it probably works well enough now. It's definitely an improvement over the status quo. (I'll try that out in a separate commit.)

@saghul
Copy link
Copy Markdown
Contributor

saghul commented Oct 26, 2024

SGTM!

@bnoordhuis bnoordhuis merged commit 89883ae into quickjs-ng:master Oct 26, 2024
@bnoordhuis bnoordhuis deleted the fix608 branch October 26, 2024 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@saghul saghul saghul approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bnoordhuis @saghul