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

chore(core): update core to C++17 #11340

Merged
merged 4 commits into from
May 14, 2024
Merged

chore(core): update core to C++17 #11340

merged 4 commits into from
May 14, 2024

Conversation

srl295
Copy link
Member

@srl295 srl295 commented May 2, 2024

Fixes: #8800

  • std::codecvt is now deprecated. Switch to ICU4C utfcodec.hpp

@keymanapp-test-bot skip

@srl295 srl295 self-assigned this May 2, 2024
@keymanapp-test-bot keymanapp-test-bot bot added the user-test-missing User tests have not yet been defined for the PR label May 2, 2024
@keymanapp-test-bot keymanapp-test-bot bot added this to the A18S1 milestone May 2, 2024
@srl295 srl295 changed the base branch from master to chore/core/11033-points-not-units May 8, 2024 20:06
@keymanapp-test-bot keymanapp-test-bot bot removed the user-test-missing User tests have not yet been defined for the PR label May 8, 2024
- use utfcodec.hpp's convert<>() instead of codecvt

Fixes: #8800
@srl295 srl295 marked this pull request as ready for review May 8, 2024 21:43
@mcdurdin
Copy link
Member

mcdurdin commented May 9, 2024

  • std::codecvt is now deprecated. Switch to ICU4C utfcodec.hpp

Good. std::codecvt was always weird

@mcdurdin
Copy link
Member

mcdurdin commented May 9, 2024

Cascade of build issues with C++17 changes sadly -- Windows, Developer both failing to build, alongside Core:Windows.

Might need to update VC++ project setting for C++ version for various projects.

Base automatically changed from chore/core/11033-points-not-units to master May 9, 2024 14:32
@srl295
Copy link
Member Author

srl295 commented May 9, 2024

@mcdurdin doesn't repro locally 😢

Build server has:

   icu-minimal| C++ compiler for the host machine: cl (msvc 19.28.29915 "Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29915 for x86")
18:09:17   icu-minimal| C++ linker for the host machine: link link 14.28.29915.0

My system has: 19.29.30146

https://learn.microsoft.com/en-us/cpp/overview/compiler-versions?view=msvc-170 says that these numbers indicate VS2019 versions 16.8-16.9 (failing) versus 16.10-16.11 (passing)

EDIT Well actually.. repro's locally in x86 but not x64 🤦

@srl295
Copy link
Member Author

srl295 commented May 9, 2024

@mcdurdin summary: works in x64, fails in x86. The project settings are updated properly. From long and .. painful experience, sounds like a compiler bug. Trying VS2022..

@mcdurdin
Copy link
Member

We could consider eliminating json from C++ code altogether -- as far as I can remember, it's only debug info anyway.

@srl295
Copy link
Member Author

srl295 commented May 10, 2024

We could consider eliminating json from C++ code altogether -- as far as I can remember, it's only debug info anyway.

it's used for writing debug info and for reading the test data for the ldml runner.

@srl295
Copy link
Member Author

srl295 commented May 10, 2024

I upgraded to VS2022 (only as a test) and it got worse - now x64 breaks too 🤦

if i upgrade to VS2024 it will make Mac stop working too (kidding)

seriously it seems like it's a known compiler issue on the json side.

@srl295
Copy link
Member Author

srl295 commented May 10, 2024

I upgraded to VS2022 (only as a test) and it got worse - now x64 breaks too 🤦

if i upgrade to VS2024 it will make Mac stop working too (kidding)

seriously it seems like it's a known compiler issue on the json side.

Perhaps a Path Forward is to keep the codecvt stuff out, but build for C++11 for now – maybe have one builder 'try' c++17 on linux to keep it working?

@mcdurdin
Copy link
Member

seriously it seems like it's a known compiler issue on the json side.

Any ref to the known issue?

Is there a #define you can use to block certain behaviours of the json lib? I haven't dug in deep but I have seen some examples

@darcywong00 darcywong00 removed this from the A18S1 milestone May 11, 2024
@darcywong00 darcywong00 added this to the A18S2 milestone May 11, 2024
- remove some tangled templates in jsonpp
- remove one unneeded include of sstream, and rewrite another as a string conversion.

Fixes: #8800
@srl295
Copy link
Member Author

srl295 commented May 13, 2024

@mcdurdin wow, the issue was not in nlohmann::json (that one was taken care of with the update done earlier), it was in jsonpp.hpp which is the non-namespaced json class.

it had some fancy templating to handle 'any' kind of string, but it ended up trying to instantiate a std::basic_string<const json & ...> (i.e. inappropriate template expansion). So rewrite to just be two specializations, one for char16 and one for char.


template<typename C>
Copy link
Member Author

Choose a reason for hiding this comment

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

i think the C here matched far too widely. ANd some of the matches "crashed" with a static assert.

Copy link
Contributor

@rc-swag rc-swag left a comment

Choose a reason for hiding this comment

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

lgtm

@srl295 srl295 merged commit 571e1fe into master May 14, 2024
17 checks passed
@srl295 srl295 deleted the chore/core/8800-cxx17 branch May 14, 2024 03:35
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.35-alpha

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

Successfully merging this pull request may close these issues.

chore(core): Update requirements to C++ 17
5 participants