Skip to content

refactor: c++ 17 #7038

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

refactor: c++ 17 #7038

wants to merge 8 commits into from
+2,250 −1,082

Conversation

ShortDevelopment
Copy link
Contributor

@ShortDevelopment ShortDevelopment commented Oct 4, 2025 •
edited
Loading

LibICU now requires c++ 17 but CC currently uses c++ 11:

ChakraCore/CMakeLists.txt

Line 375 in 36becec

set(CMAKE_CXX_STANDARD 11)

This causes MacOS ci to fail.

📣 Changes

⚠️ Deprecations

💥 Breaking changes

  • ...

@ppenzin

Fixes #3147
Fixes #6378

@ShortDevelopment ShortDevelopment changed the title [WIP] chore: upgrade to c++ 17 [WIP] refactor: c++ 17 Oct 4, 2025
@ShortDevelopment ShortDevelopment changed the title [WIP] refactor: c++ 17 refactor: c++ 17 Oct 4, 2025
@rhuanjl
Copy link
Collaborator

rhuanjl commented Oct 8, 2025

Is this one next up? Do you know what caused the test fails?

@ShortDevelopment
Copy link
Contributor Author

@rhuanjl I updated the test framework used in NativeTests. I still need some time to fix this.
Could you have a look at #7042 in the meantime?

rhuanjl
rhuanjl reviewed Oct 10, 2025
View reviewed changes
lib/Runtime/Language/JavascriptExceptionOperators.h Outdated
#elif defined(_M_ARM)
extern "C" void *arm_CallEhFrame(void *target, void *framePtr, void *localsPtr, size_t argsSize) throw(...);
extern "C" void *arm_CallCatch(void *target, void *framePtr, void *localsPtr, size_t argsSize, void *catchObj) throw(...);
extern "C" void *arm_CallEhFrame(void *target, void *framePtr, void *localsPtr, size_t argsSize);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be a noexcept(false) on this line, like the other lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yes. Seems like I missed that one.
Interestingly, none of the CI compilations got that...

Copy link
Collaborator

Choose a reason for hiding this comment

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

32bit arm is a gap in our testing coverage; when we set up the CI we couldn't find a free service that would test it for us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 6bf3471

@ShortDevelopment
Copy link
Contributor Author

ShortDevelopment commented Oct 10, 2025 •
edited
Loading

We still get a test failure for x64 NativeTests which likely means GC is not working any more (not good).

ChakraCore/bin/NativeTests/JsRTApiTest.cpp

Line 136 in c6a9549

CHECK(valueRefAfterGC == JS_INVALID_REFERENCE);

So far, I cannot reproduce this issue with a local build.

Edit: The issue is reproducable using the ci build from Azure Artifacts.
The issues seems to be within ChakraCore.dll (using my local build of ChakraCore.dll fixes the issue)

Edit: Only happens in 'Test' (likely in 'Release' aswell) not 'Debug' configuration.

@rhuanjl
Copy link
Collaborator

rhuanjl commented Oct 10, 2025

We still get a test failure for x64 NativeTests which likely means GC is not working any more (not good).

ChakraCore/bin/NativeTests/JsRTApiTest.cpp

Line 136 in c6a9549

CHECK(valueRefAfterGC == JS_INVALID_REFERENCE);

So far, I cannot reproduce this issue with a local build.

Edit: The issue is reproducable using the ci build from Azure Artifacts. The issues seems to be within ChakraCore.dll (using my local build of ChakraCore.dll fixes the issue)

Edit: Only happens in 'Test' (likely in 'Release' aswell) not 'Debug' configuration.

Can you reproduce with a local Test build? Or only by downloading a test build? If this dependent on compiler versions it may be awkward to solve, hopefully it's a simple configuration issue somehow...

@ShortDevelopment
Copy link
Contributor Author

Can you reproduce with a local Test build?

That's the problem: I can't 🙈
(I can with the x64 Test binaries from Azure Artifacts though)

If this dependent on compiler versions it may be awkward to solve

I fear that might be the case...

hopefully it's a simple configuration issue somehow...

Here's what I did:

  • Download zip of this branch from github
  • Unzip
  • Open VS Command-Prompt (Vs 2022 v17.14.16)
  • .\test\ci.buildone.cmd x64 test
  • .\test\runnativetests.cmd -d true -x64 -test
  • All Tests pass

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rhuanjl rhuanjl rhuanjl left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

C++20 removed std::uncaught_exception Msvc with ‘/std:c++latest’: std::uncaught_exception() is deprecated in C++17

2 participants

@ShortDevelopment @rhuanjl