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

Bump c++ version to 20 #627

Closed
wants to merge 3 commits into from
Closed

Bump c++ version to 20 #627

wants to merge 3 commits into from

Conversation

Cropi
Copy link
Member

@Cropi Cropi commented Jun 4, 2024

Currently, the CI is failing because it's not able to parse the new [[ unlikely ]] attribute. I am not aware of any other new features the project uses but it might use in the future. We can either remove the attribute or switch to c++20.

Considering that c++20 has been out for a while and compilers already support it, I think it's safe to do.
More info: https://en.cppreference.com/w/cpp/20

@Cropi
Copy link
Member Author

Cropi commented Jun 4, 2024

Hello @hartwork,
You as the main contributor to our CI, do you have an opinion on this topic?

  • switch to c++20 and fix the CI by using the latest OS or
  • remove c++20 features and use the current setup

Thank you in advance.

@hartwork
Copy link
Contributor

hartwork commented Jun 4, 2024

@Cropi thanks for the notification. C++20 is younger than 5 years and so requiring C++20 for a software that (1) can easily do without it and (2) is crucial to non-bleeding-edge enterprise I would like to clearly vote against requiring C++20. If you want to make that move, in my view it first needs a list of must-support systems and checking their versions of GCC and Clang and checking the C++20 support of those compilers. Questions like "what is the oldest release of RHEL, Debian, SUSE, Ubuntu that we need to support?" It takes work and before that work the risk is too high in my book.

On a side note I find it concerning that pull request #600 was merged to master despite failing CI. Getting red CI fixed should be everyone's first priority before merging anything else.

What do you think?

@hartwork
Copy link
Contributor

hartwork commented Jun 4, 2024

@Cropi I have fixes for all issues with CI now in pull request #628. I am looking forward to your review 🙏

@Cropi
Copy link
Member Author

Cropi commented Jun 5, 2024

@Cropi thanks for the notification. C++20 is younger than 5 years and so requiring C++20 for a software that (1) can easily do without it and (2) is crucial to non-bleeding-edge enterprise I would like to clearly vote against requiring C++20. If you want to make that move, in my view it first needs a list of must-support systems and checking their versions of GCC and Clang and checking the C++20 support of those compilers. Questions like "what is the oldest release of RHEL, Debian, SUSE, Ubuntu that we need to support?" It takes work and before that work the risk is too high in my book.

Yes, I fully agree here. It might be an overkill to switch to a newer version when there are no big features depending on it. Thanks for the quick reaction.

On a side note I find it concerning that pull request #600 was merged to master despite failing CI. Getting red CI fixed should be everyone's first priority before merging anything else.

What do you think?

@Cropi Cropi closed this Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants