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

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
+2,258 −1,082
Draft

refactor: c++ 17 #7038

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b00bba6
Set cpp standard to 17
ShortDevelopment Oct 4, 2025
e77bbfe
Remove deprecated exception spec
ShortDevelopment Oct 4, 2025
87323ae
Fix styling
ShortDevelopment Oct 4, 2025
a1842fe
Upgrade to catch v1.12.2
ShortDevelopment Oct 4, 2025
177a3e0
`std::uncaught_exception` deprecated
ShortDevelopment Oct 4, 2025
cd81a22
Use `icu4c` on MacOS again
ShortDevelopment Oct 9, 2025
6bf3471
Add missing `noexcept(false)`
ShortDevelopment Oct 10, 2025
b8e02b3
Move msbuild `LanguageStandard` to existing structure
ShortDevelopment Oct 10, 2025
3f72f99
Try win25 for x64 Tests
ShortDevelopment Oct 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .cirrus.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ task:
macos_instance:
image: ghcr.io/cirruslabs/macos-ventura-xcode
Dependencies_script: brew install ninja icu4c && mkdir -p build
CMake_script: cd build && cmake -GNinja -DCMAKE_BUILD_TYPE=Debug -DSTATIC_LIBRARY=ON -DEMBED_ICU=ON -DDISABLE_JIT=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang ..
CMake_script: cd build && cmake -GNinja -DCMAKE_BUILD_TYPE=Debug -DSTATIC_LIBRARY=ON -DICU_INCLUDE_PATH=/opt/homebrew/opt/icu4c/include -DDISABLE_JIT=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang ..
Build_script: cd build && ninja
Test_script: cd build && ninja check

Expand All @@ -17,7 +17,7 @@ task:
macos_instance:
image: ghcr.io/cirruslabs/macos-ventura-xcode
Dependencies_script: brew install ninja icu4c && mkdir -p build
CMake_script: cd build && cmake -GNinja -DCMAKE_BUILD_TYPE=RelWithDebInfo -DEMBED_ICU=ON -DDISABLE_JIT=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang ..
CMake_script: cd build && cmake -GNinja -DCMAKE_BUILD_TYPE=RelWithDebInfo -DICU_INCLUDE_PATH=/opt/homebrew/opt/icu4c/include -DDISABLE_JIT=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang ..
Build_script: cd build && ninja
Test_script: cd build && ninja check

Expand All @@ -26,6 +26,6 @@ task:
macos_instance:
image: ghcr.io/cirruslabs/macos-ventura-xcode
Dependencies_script: brew install ninja icu4c && mkdir -p build
CMake_script: cd build && cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DSTATIC_LIBRARY=ON -DEMBED_ICU=ON -DDISABLE_JIT=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang ..
CMake_script: cd build && cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DSTATIC_LIBRARY=ON -DICU_INCLUDE_PATH=/opt/homebrew/opt/icu4c/include -DDISABLE_JIT=ON -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_C_COMPILER=clang ..
Build_script: cd build && ninja
Test_script: cd build && ninja check
2 changes: 1 addition & 1 deletion .editorconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,5 @@ cpp_new_line_before_catch = true
cpp_new_line_before_else = true

# Xml files
[*.xml]
[*.{xml,props}]
indent_size = 2
2 changes: 1 addition & 1 deletion Build/Common.Build.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@
<PreprocessorDefinitions Condition="'$(ChakraVersionBuildCommit)'!=''">%(PreprocessorDefinitions);CHAKRA_VERSION_BUILD_COMMIT=$(ChakraVersionBuildCommit)</PreprocessorDefinitions>
<PreprocessorDefinitions Condition="'$(ChakraVersionBuildDate)'!=''">%(PreprocessorDefinitions);CHAKRA_VERSION_BUILD_DATE=$(ChakraVersionBuildDate)</PreprocessorDefinitions>


<LanguageStandard>stdcpp17</LanguageStandard>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to specify this? I can't see that we did before.

(Also wondering if for some reason this is causing our build failure)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CC on Windows uses MsBuild instead of CMake as build system. Therefore I need to set the new language standard in 2 places.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm aware of the separate build system, my point was that prior to this PR whereas in cmake we specified a language standard, in msbuild we did not - unless it was set some other way I've not seen.

I was conjecturing if explicitly specifying it on the version of windows used in the CI was causing a problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looking at the diff and the failure options seem to be:

  1. Somehow specifying the language standard has resulted in different behaviour - e.g. there's a C++ breaking change OR a compiler bug that we're falling into.
  2. The new version of the Catch framework isn't working.
  3. This is something to do with those throw(...) markers being changed to no_except; (this seems unlikely to me).

Awkwardly most of the native tests do not run on Linux/macOS so it'ss possible this problem is occurring on those platforms too (if this issue is a breaking change to C++).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used BinDiff to diff the last "working" version from ci and the ci version from this PR. The only obvious thing was the compiler inlining about 39 more methods (for win-x64).
But linux might be broken too (See #7038 (comment))

</ClCompile>
<ResourceCompile>
<PreprocessorDefinitions Condition="'$(ChakraVersionBuildNumber)'!=''">%(PreprocessorDefinitions);CHAKRA_VERSION_BUILD_NUMBER=$(ChakraVersionBuildNumber)</PreprocessorDefinitions>
Expand Down
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ if(CLR_CMAKE_PLATFORM_XPLAT)
-D__STDC_WANT_LIB_EXT1__=1
)

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD 17)
set(CMAKE_CXX_STANDARD_REQUIRED On)

# todo: fix general visibility of the interface
Expand Down
12 changes: 10 additions & 2 deletions azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,12 @@ jobs:
image_name: 'macOS-latest'
deps: 'brew install ninja icu4c'
build_type: 'RelWithDebInfo'
libtype_flag: '-DEMBED_ICU=ON'
libtype_flag: '-DICU_INCLUDE_PATH=/usr/local/opt/icu4c/include'
OSX.Release:
image_name: 'macOS-latest'
deps: 'brew install ninja icu4c'
build_type: 'Release'
libtype_flag: '-DEMBED_ICU=ON'
libtype_flag: '-DICU_INCLUDE_PATH=/usr/local/opt/icu4c/include'

pool:
vmImage: $(image_name)
Expand Down Expand Up @@ -163,6 +163,14 @@ jobs:
do_test: true
test_tags: '--include-slow'
build_outdir_suffix: ''
win25.x64.Test:
image_name: 'windows-2025'
build_type: 'test'
target: 'x64'
special_build: ''
do_test: true
test_tags: '--include-slow'
build_outdir_suffix: ''
x64.Release:
image_name: 'windows-2022'
build_type: 'release'
Expand Down
Loading
Loading