[core] update to c++20 #7299

RedAuburn · 2024-02-02T14:52:53Z

RedAuburn commented

2024-02-02 14:52:53 +00:00

Seems to be all that's needed to upgrade.

Using g++13 because it's the first version with support for std::format (and close to full support for c++20 generally afaik) https://stackoverflow.com/a/69156536

building android & desktop works

Important limitations on different iOS and XCode versions for some C++17 and above features support

https://developer.apple.com/xcode/cpp/

Seems to be all that's needed to upgrade. Using g++13 because it's the first version with support for std::format (and close to full support for c++20 generally afaik) https://stackoverflow.com/a/69156536 building android & desktop works ## Important limitations on different iOS and XCode versions for some C++17 and above features support https://developer.apple.com/xcode/cpp/

👍 1

vng (Migrated from github.com) reviewed 2024-02-02 17:29:31 +00:00

docs/CPP_STYLE.md

vng (Migrated from github.com) commented

2024-02-02 17:29:06 +00:00

Let's give a respect to our fav language :) Write with capital C++20 here and below.

vng (Migrated from github.com) reviewed 2024-02-02 17:31:15 +00:00

CMakeLists.txt

vng (Migrated from github.com) commented

2024-02-02 17:31:15 +00:00

Is this comment from some future PR?

RedAuburn reviewed 2024-02-02 18:21:26 +00:00

CMakeLists.txt

RedAuburn commented

2024-02-02 18:21:26 +00:00

yes 😅 i guess it could be added in that PR, but i thought it was just easier to add it now

RedAuburn reviewed 2024-02-02 18:21:29 +00:00

docs/CPP_STYLE.md

RedAuburn commented

2024-02-02 18:21:29 +00:00

fixed :)

Ferenc- (Migrated from github.com) approved these changes 2024-03-19 14:17:29 +00:00

pastk approved these changes 2024-03-19 18:07:07 +00:00

biodranik (Migrated from github.com) requested changes 2024-03-26 23:19:27 +00:00

biodranik (Migrated from github.com) left a comment

I'm putting it on hold until the crash is fixed and the PR is tested.

What is the minimum gcc version that will work with our code on this branch?

I'm putting it on hold until the crash is fixed and the PR is tested. What is the minimum gcc version that will work with our code on this branch?

biodranik (Migrated from github.com) reviewed 2024-05-01 11:18:09 +00:00

.github/workflows/linux-check.yaml

biodranik (Migrated from github.com) commented

2024-05-01 11:18:09 +00:00

Can compiler be updated independently from switching the code to C++20?

biodranik (Migrated from github.com) reviewed 2024-05-01 11:18:55 +00:00

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-05-01 11:18:55 +00:00

Did anyone test std::format already on iOS 12 and Android 5 devices?

Osyotr (Migrated from github.com) reviewed 2024-05-01 11:27:35 +00:00

.github/workflows/linux-check.yaml

Osyotr (Migrated from github.com) commented

2024-05-01 11:27:35 +00:00

I'd say there's no reason to update GCC in this PR and Clang 15 is still too old anyways.

Jean-BaptisteC (Migrated from github.com) approved these changes 2024-05-01 12:34:01 +00:00

Jean-BaptisteC (Migrated from github.com) left a comment

✅ Pixel 6 - Android 14

Ferenc- (Migrated from github.com) reviewed 2024-05-17 09:35:23 +00:00

.github/workflows/linux-check.yaml

Ferenc- (Migrated from github.com) commented

2024-05-17 09:35:23 +00:00

gcc 14 and clang 18 are now in master. @RedAuburn please consider a rebase

`gcc 14` and `clang 18` are now in `master`. @RedAuburn please consider a rebase

biodranik (Migrated from github.com) reviewed 2024-05-17 11:51:57 +00:00

.github/workflows/linux-check.yaml

biodranik (Migrated from github.com) commented

2024-05-17 11:51:57 +00:00

@Ferenc- this is an incorrect statement ) gcc 14 and clang 18 are enabled on Github CI is the correct one.

@Ferenc- this is an incorrect statement ) `gcc 14 and clang 18 are enabled on Github CI` is the correct one.

RedAuburn reviewed 2024-05-27 11:43:54 +00:00

CMakeLists.txt

RedAuburn commented

2024-05-27 11:43:53 +00:00

this is correct, right?

biodranik (Migrated from github.com) reviewed 2024-05-27 20:38:52 +00:00

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-05-27 20:38:52 +00:00

No, please check this table, and it would be great to also test on different GCC versions. Highly likely that gcc-10 or 11 would be ok. You can experiment locally in this PR by adding other gcc version matrix values in the .github workflow check.

No, please check [this table](https://gcc.gnu.org/projects/cxx-status.html), and it would be great to also test on different GCC versions. Highly likely that gcc-10 or 11 would be ok. You can experiment locally in this PR by adding other gcc version matrix values in the .github workflow check.

Osyotr (Migrated from github.com) reviewed 2024-05-28 16:13:04 +00:00

docs/CPP_STYLE.md

					
				@ -14,2 +14,2 @@

				- We are using all features of C++17 except the filesystem which is not fully supported on all platforms.

				- We try to limit the usage of boost libraries which require linking (and prefer C++17 types over their boost counterparts).

				- We are using all features of C++17 and C++20 except std::filesystem, std::to_chars & std::from_chars which are not fully supported on all platforms.

				- We try to limit the usage of boost libraries which require linking (and prefer C++20 types over their boost counterparts).

Osyotr (Migrated from github.com) commented

2024-05-28 16:13:04 +00:00

Unfortunately, the filesystem remark is still relevant.
organicmaps/organicmaps#8256 (comment)

Unfortunately, the filesystem remark is still relevant. https://git.omaps.dev/organicmaps/organicmaps/pulls/8256#issuecomment-2134054267

RedAuburn reviewed 2024-05-28 16:16:14 +00:00

docs/CPP_STYLE.md

					
				@ -14,2 +14,2 @@

				- We are using all features of C++17 except the filesystem which is not fully supported on all platforms.

				- We try to limit the usage of boost libraries which require linking (and prefer C++17 types over their boost counterparts).

				- We are using all features of C++17 and C++20 except std::filesystem, std::to_chars & std::from_chars which are not fully supported on all platforms.

				- We try to limit the usage of boost libraries which require linking (and prefer C++20 types over their boost counterparts).

RedAuburn commented

2024-05-28 16:16:14 +00:00

Damnit apple, will update

RedAuburn reviewed 2024-05-28 17:12:29 +00:00

CMakeLists.txt

RedAuburn commented

2024-05-28 17:12:29 +00:00

checked, gcc-9 fails but gcc-10 works, have updated PR 👍️

biodranik (Migrated from github.com) reviewed 2024-05-28 18:39:42 +00:00

docs/CPP_STYLE.md

					
				@ -14,2 +14,2 @@

				- We are using all features of C++17 except the filesystem which is not fully supported on all platforms.

				- We try to limit the usage of boost libraries which require linking (and prefer C++17 types over their boost counterparts).

				- We are using all features of C++17 and C++20 except std::filesystem, std::to_chars & std::from_chars which are not fully supported on all platforms.

				- We try to limit the usage of boost libraries which require linking (and prefer C++20 types over their boost counterparts).

biodranik (Migrated from github.com) commented

2024-05-28 18:39:42 +00:00

Elementary string conversions std::to_chars, std::from_chars is also not available.

RedAuburn reviewed 2024-05-28 23:11:05 +00:00

docs/CPP_STYLE.md

					
				@ -14,2 +14,2 @@

				- We are using all features of C++17 except the filesystem which is not fully supported on all platforms.

				- We try to limit the usage of boost libraries which require linking (and prefer C++17 types over their boost counterparts).

				- We are using all features of C++17 and C++20 except std::filesystem, std::to_chars & std::from_chars which are not fully supported on all platforms.

				- We try to limit the usage of boost libraries which require linking (and prefer C++20 types over their boost counterparts).

RedAuburn commented

2024-05-28 23:11:05 +00:00

noted, thanks

biodranik (Migrated from github.com) reviewed 2024-05-28 23:11:31 +00:00

biodranik (Migrated from github.com) left a comment

Thanks, LGTM. Let's test it before the merge in alpha/beta.

CC @vng @rtsisyk

Thanks, LGTM. Let's test it before the merge in alpha/beta. CC @vng @rtsisyk

RedAuburn reviewed 2024-05-29 09:50:36 +00:00

CMakeLists.txt

RedAuburn commented

2024-05-29 09:50:36 +00:00

removed

Ferenc- (Migrated from github.com) reviewed 2024-05-31 14:56:46 +00:00

CMakeLists.txt

Ferenc- (Migrated from github.com) commented

2024-05-31 14:56:46 +00:00

How do we expect to maintain this minimum 10.0, if we don't even test in the CI with 10.0?
Next PR going in after this, can add something, which suddenly needs 14.0 and nobody even notices, because that still passes the CI.

How do we expect to maintain this minimum `10.0`, if we don't even test in the CI with `10.0`? Next PR going in after this, can add something, which suddenly needs `14.0` and nobody even notices, because that still passes the CI.

Osyotr (Migrated from github.com) reviewed 2024-05-31 15:31:04 +00:00

CMakeLists.txt

Osyotr (Migrated from github.com) commented

2024-05-31 15:31:04 +00:00

Suggestion: remove this check altogether.

biodranik (Migrated from github.com) reviewed 2024-05-31 20:27:56 +00:00

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-05-31 20:27:55 +00:00

This check will save us and other contributors time debugging build issues when older compilers are used.

We can switch from clang-18 to gcc-10 in the Linux check Linux no unity build job.

This check will save us and other contributors time debugging build issues when older compilers are used. We can switch from clang-18 to gcc-10 in the Linux check `Linux no unity build` job.

Osyotr (Migrated from github.com) reviewed 2024-05-31 21:21:21 +00:00

CMakeLists.txt

Osyotr (Migrated from github.com) commented

2024-05-31 21:21:21 +00:00

You can express your usage requirements in CMake using target_compile_features together with things from CMAKE_CXX_KNOWN_FEATURES.
That way you don't need to check anything - CMake will do it for you.

You can express your usage requirements in CMake using [target_compile_features](https://cmake.org/cmake/help/latest/manual/cmake-compile-features.7.html) together with things from [CMAKE_CXX_KNOWN_FEATURES](https://cmake.org/cmake/help/latest/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html). That way you don't need to check anything - CMake will do it for you.

biodranik (Migrated from github.com) reviewed 2024-05-31 21:28:23 +00:00

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-05-31 21:28:23 +00:00

What exactly do you propose to add?

Osyotr (Migrated from github.com) reviewed 2024-05-31 21:35:46 +00:00

CMakeLists.txt

Osyotr (Migrated from github.com) commented

2024-05-31 21:35:46 +00:00

I don't propose to add anything, I just don't think this check solves any real problem.

biodranik (Migrated from github.com) reviewed 2024-05-31 21:52:52 +00:00

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-05-31 21:52:52 +00:00

Waste of time is a real problem, especially at scale.

Waste of time is a _real_ problem, especially at scale.

Ferenc- (Migrated from github.com) reviewed 2024-06-03 11:03:54 +00:00

CMakeLists.txt

Ferenc- (Migrated from github.com) commented

2024-06-03 11:03:54 +00:00

Unfurtunately CMAKE_CXX_KNOWN_FEATURES, doesn't have individual feature support for C++20, and even if it had I don't think it could really subsitute actually testing, and currently we only test something that we could call "upper bound" compilers.
I propose to either split or change the linux-matrix, so it tests lower bound and upper bound compilers, not just the upper bound as it is currently.
In any case line 169 # GCC 8.1 is required should be changed in this PR.

Unfurtunately `CMAKE_CXX_KNOWN_FEATURES`, doesn't have individual feature support for C++20, and even if it had I don't think it could really subsitute actually testing, and currently we only test something that we could call "upper bound" compilers. I propose to either split or change the [linux-matrix](https://github.com/organicmaps/organicmaps/blob/master/.github/workflows/linux-check.yaml#L96), so it tests lower bound and upper bound compilers, not just the upper bound as it is currently. In any case line 169 `# GCC 8.1 is required` should be changed in this PR.

biodranik (Migrated from github.com) reviewed 2024-06-03 13:21:55 +00:00

biodranik (Migrated from github.com) left a comment

Let's finish and merge this PR, looks like it has passed alpha testing. There is one report about strange crashes on Android, it is not clear yet if it is related to this or other changes.

CMakeLists.txt

biodranik (Migrated from github.com) commented

2024-06-03 13:20:54 +00:00

# GCC 10.0 is required to support <charconv> header inclusion in base/string_utils.hpp

Is GCC version mentioned somewhere in the Linux build docs? Should we mention it explicitly there too?

```suggestion # GCC 10.0 is required to support <charconv> header inclusion in base/string_utils.hpp ``` Is GCC version mentioned somewhere in the Linux build docs? Should we mention it explicitly there too?