-
Notifications
You must be signed in to change notification settings - Fork 329
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
Update project to require C++17 #1241
Comments
We have updated P4C to C++17 a while ago. I think requiring C++17 at minimum is fair. |
We should check with @smolkaj if that's acceptable to him, but if it's fine for p4c it should be fine for bmv2 as well. |
Yes, the newer the better from my side. |
Hello , I would like to take this up as my very first contribution to get into open source. |
The pragmatic answer is that you simply switch this flag here: https://github.com/p4lang/behavioral-model/blob/main/configure.ac#L130 and then try to compile the behavioral model. Things will likely break and you may have to fix the issues one after the other. |
@OsamaReb3 I saw an email that appears to be a question from you for Fabian, but when I try to reply-all to that email, your email address is not obviously in the list of recipients, so I am responding this way instead. To create a pull request, you do not need any account on any web site except github.com, which it appears you already do. The basics are:
Before your pull request can be merged in, it must be approved by others. Currently, you must also have digitally signed the Open Networking Foundation CLA. But let's worry about how to jump through that hoop after you get through the steps above. |
@jafingerhut I suspect someone deleted the comment as it is unrelated to the issue |
I've made the changes to the build system and documentation to reflect this update. Could you please guide me on the best practices for testing these changes? Specifically, I want to ensure that the upgrade does not introduce any issues and that the project remains stable and functional. |
Just open a pull request to this repository. https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/proposing-changes-to-your-work-with-pull-requests/creating-a-pull-request The continuous integration will automatically test your changes and we can review them. |
For #1241 Signed-off-by: osama <[email protected]>
The project has been requiring / supporting C++11 since its inception.
It think it may be time to bump this requirement up to C++14, so that contributors can start using features introduced in C++14. I would personally be fine with C++17, but C++14 is more conservative.
One immediate advantage would be to remove the
boost/thread/shared_mutex.hpp
dependency, since C++14 includes a shared mutex (With support for RW lock) as part of the standard library.Most of the required changes would be targeted at the build system and documentation.
Edit: based on discussion below, C++17 seems preferred to C++14. So that should be the plan unless there is a timely objection from someone.
The text was updated successfully, but these errors were encountered: