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.

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

[core] update to c++20 #7299

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[core] update to c++20 #7299

wants to merge 1 commit into from

Conversation

RedAuburn
Copy link
Sponsor Member

@RedAuburn RedAuburn commented Feb 2, 2024 •
edited

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/

@RedAuburn
Copy link
Sponsor Member Author

RedAuburn commented Feb 2, 2024 •
edited

Ah, need to update the github runners.

upd:
should have fixed linux, but not sure how to do ios/mac
need to use xcode 15.3 (https://developer.apple.com/xcode/cpp/#c++20) for std::format

@vng vng requested a review from pastk February 2, 2024 17:25
@vng
Copy link
Member

vng commented Feb 2, 2024

@pastk @Ferenc- Merge only after yours approve. Please, check at least the compilation. You have some fancy/old Linux distros, AFAIK.

@vng vng requested a review from Ferenc- February 2, 2024 17:27
vng
vng reviewed Feb 2, 2024
View reviewed changes
docs/CPP_STYLE.md Outdated Show resolved Hide resolved
vng
vng reviewed Feb 2, 2024
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@RedAuburn
Copy link
Sponsor Member Author

fixed capitalisation in docs

@AndrewShkrob AndrewShkrob added the Coverage Add this label to your PR if you want to run coverage check label Feb 2, 2024
@AndrewShkrob
Copy link
Member

I've added the label Coverage to ensure that you remember to fix this runner as well :)

@biodranik
Copy link
Member

After starting using any XCode 15 feature we break the option to build and run XCode 14 with iOS 12 simulator.

@pastk
Copy link
Contributor

pastk commented Feb 3, 2024

Using g++13 because it't the first version with support for std::format

GCC 13.1 was released last April, so its super new, e.g. its not the default in the current Ubuntu 22.04 LTS.

There are custom backports of GCC 12 to my very dated system, but not 13 (though maybe I didn't search thoroughly enough).

Anyway its very likely there will be building issues on many older systems.

My understanding is that its better to find some alternative to std::format support...

@RedAuburn
Copy link
Sponsor Member Author

hmm yeah it's sounding like an update to c++20 might be more trouble than it's worth

@RedAuburn RedAuburn closed this Feb 3, 2024
@AndrewShkrob
Copy link
Member

I don't observe any correlation between C++20 and std::format.

If the intention is to update to C++20, please proceed with the update.
Alternatively, if the goal is to utilize std::format, consider using fmtlib.

@biodranik
Copy link
Member

Let's make a safe list of C++20 features that are already available on most of our operating systems, that we can benefit from, and save our development time. Enabling C++20 compilation doesn't mean that we are forced to use its features. We can choose only some convenient one/important one that help us to write cleaner code.

@biodranik biodranik reopened this Feb 3, 2024
@biodranik
Copy link
Member

@RedAuburn is snprintf enough for your task?

@AndrewShkrob
Copy link
Member

AndrewShkrob commented Feb 3, 2024 •
edited

Let's make a safe list of C++20 features that are already available on most of our operating systems.

@biodranik please, do not overcomplicate. We have github runners to do this work for us:

  • The user creates a PR
  • Runner fails
  • Feature is not supported
  • Fix with c++17 features

If some of our regular contributors use outdated gcc/libs, then I suggest them to use our "official" supported gcc/libs from runners.
Or if that's not possible (which I doubt) we can add an additional configuration for runners to utilize their gcc/libs.

Btw, there is already such list: https://en.cppreference.com/w/cpp/20
And for C++23 (why not to update to it?): https://en.cppreference.com/w/cpp/23

@Ferenc-
Copy link
Contributor

Ferenc- commented Feb 4, 2024

There are custom backports of GCC 12 to my very dated system, but not 13 (though maybe I didn't search thoroughly enough).

✔️ I have checked it is also available for your 18.04:

sudo add-apt-repository ppa:ubuntu-toolchain-r/test
sudo apt update
sudo apt install gcc-13 g++-13
sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-13 10 --slave /usr/bin/g++ g++ /usr/bin/g++-13

Anyway its very likely there will be building issues on many older systems.

✔️ So far I haven't found any extra build issues, that would be tied to older systems.
We have been providing build instructions in the past in the documentation and we can continue to do so in the future.
Furthermore there is the option to build in a container / flatpak.

@RedAuburn would it be an option to keep this PR branch on top of the current upstream master? For example one fix pertaining to the desktop build is already there. Some more will be needed for the tests.

@biodranik
Copy link
Member

@AndrewShkrob did you see the table for iOS? Just build error is not enough, there are also some runtime OS dependencies in some cases.

@pastk
Copy link
Contributor

pastk commented Feb 5, 2024

✔️ I have checked it is also available for your 18.04:

Many thanks, I'll give it a go later when I have time!

@RedAuburn
Copy link
Sponsor Member Author

(rebased)

Ferenc- added a commit to Ferenc-/organicmaps that referenced this pull request Feb 5, 2024
@Ferenc-
[base] Add C++20 char16_t support to DebugPrint
c39d616 
This change is needed for organicmaps#7299.

C++20 deletes a number of operators including
`operator<<(basic_ostream<char, _Traits>&, char16_t) = delete;`.
And consequentely the compilation with `--std=c++20`
fails with `error: use of deleted function` on `out << t;`.
Hence the generic
`template <typename T> inline std::string DebugPrint(T const & t)`
needs to be specialized for `char16_t`.
For our purposes, we reuse the built-in `formatter` for `char` in this change.

Note, that simply initializing a string like `{1, t}` would emmit:
`warning: narrowing conversion of ‘t’ from ‘char16_t’ to ‘char’`

Signed-off-by: Ferenc Géczi <ferenc.gm@gmail.com>
This was referenced Feb 5, 2024
Closed
Merged
Ferenc- added a commit to Ferenc-/organicmaps that referenced this pull request Feb 6, 2024
@Ferenc-
[base] Add C++20 char16_t support to DebugPrint
8334dfe 
This change is needed for organicmaps#7299.

C++20 deletes a number of operators including
`operator<<(basic_ostream<char, _Traits>&, char16_t) = delete;`.
And consequentely the compilation with `--std=c++20`
fails with `error: use of deleted function` on `out << t;`.
Hence the generic
`template <typename T> inline std::string DebugPrint(T const & t)`
needs to be specialized for `char16_t`.

Signed-off-by: Ferenc Géczi <ferenc.gm@gmail.com>
@Ferenc-
Copy link
Contributor

Ferenc- commented Feb 8, 2024

@RedAuburn please rebase again to upstream master, there is one needed change there.
After that please consider cherry-pick -ing this commit from me, which is needed because of a runner issue (actions/runner-images#8659). After all that the CI should be passing ✔️

@biodranik
Copy link
Member

There is also android/ndk#1981

And this note:
Note: For full details of the expected level of C++ library support for any given version, see the C++14 Status, C++17 Status, and C++20 Status pages. (C++20 was previously known as C++2a.) The level of C++ language support in the compiler is orthogonal; see C++ Support in Clang instead.

C++ locale class, for example, was not supported from the beginning on Android. What else is missing?

Note that there's no static runtime on iOS, so that looks like the weakest spot now with iOS 12.

@biodranik biodranik added this to the Needs alpha/beta testing milestone May 27, 2024
@biodranik
Copy link
Member

biodranik commented May 27, 2024 •
edited

Please rebase, and let's alpha-test it, need to make sure that iOS 12/13 and Android 5+ work on the current code.

Important iOS limitations: https://developer.apple.com/xcode/cpp/

RedAuburn
RedAuburn commented May 27, 2024
View reviewed changes
CMakeLists.txt Outdated Show resolved Hide resolved
@kirylkaveryn
Copy link
Contributor

Please rebase, and let's alpha-test it, need to make sure that iOS 12/13 and Android 5+ work on the current code.

Important iOS limitations: https://developer.apple.com/xcode/cpp/

I've tested this PR on ios 12.5 and didn't encounter any issues.

@RedAuburn
Copy link
Sponsor Member Author

fixing coverage check in #8308 (unrelated to this pr)

@RedAuburn
Copy link
Sponsor Member Author

working fine on Android 5.1 (Oneplus One)

@RedAuburn RedAuburn removed the Coverage Add this label to your PR if you want to run coverage check label May 28, 2024
@RedAuburn RedAuburn force-pushed the to-cpp20 branch 4 times, most recently from 7455342 to b387840 Compare May 28, 2024 16:31
@RedAuburn
Copy link
Sponsor Member Author

gcc 10 works

@RedAuburn
Copy link
Sponsor Member Author

9.0 failed, going with minversion of 10

@RedAuburn RedAuburn added the Coverage Add this label to your PR if you want to run coverage check label May 28, 2024
@RedAuburn
Copy link
Sponsor Member Author

RedAuburn commented May 28, 2024 •
edited

Should be good to test/merge finally :)

biodranik
biodranik reviewed May 28, 2024
View reviewed changes
Copy link
Member

@biodranik biodranik left a comment

Choose a reason for hiding this comment

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

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

CC @vng @rtsisyk

@RedAuburn
Copy link
Sponsor Member Author

added info that std::to_chars & std::from_chars aren't available to docs

@RedAuburn
Copy link
Sponsor Member Author

accidentally messed up dco signature, fixed

@RedAuburn
Update to C++20
2d02652 
Signed-off-by: Harry Bond <me@hbond.xyz>
Ferenc-
Ferenc- reviewed May 31, 2024
View reviewed changes
CMakeLists.txt
message(FATAL_ERROR "Minimum supported g++ version is 8.1 yours is ${CMAKE_CXX_COMPILER_VERSION}")
# GCC 8.1 is required to support <charconv> header inclusion in base/string_utils.hpp
if (CMAKE_CXX_COMPILER_ID MATCHES "GNU" AND CMAKE_CXX_COMPILER_VERSION VERSION_LESS 10.0)
message(FATAL_ERROR "Minimum supported g++ version is 10.0, yours is ${CMAKE_CXX_COMPILER_VERSION}")
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: remove this check altogether.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

What exactly do you propose to add?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

@biodranik biodranik biodranik requested changes

@vng vng vng left review comments

@Ferenc- Ferenc- Ferenc- approved these changes

@Osyotr Osyotr Osyotr left review comments

@pastk pastk pastk approved these changes

@Jean-BaptisteC Jean-BaptisteC Jean-BaptisteC approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Coverage Add this label to your PR if you want to run coverage check
Projects
None yet
Milestone
Ready for alpha testing
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@RedAuburn @vng @AndrewShkrob @biodranik @pastk @Ferenc- @kirylkaveryn @Osyotr @Jean-BaptisteC