search

p4lang / behavioral-model

The reference P4 software switch
Apache License 2.0
531 stars 327 forks source link

Update project to require C++17 #1241

Open antoninbas opened 2 months ago

antoninbas commented 2 months ago

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.

fruffy commented 2 months ago

We have updated P4C to C++17 a while ago. I think requiring C++17 at minimum is fair.

antoninbas commented 2 months ago

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.

smolkaj commented 2 months ago

Yes, the newer the better from my side. We're on C++20 internally at Google.

OsamaRab3 commented 1 month ago

Hello ,

I would like to take this up as my very first contribution to get into open source. I've never done this before and would really appreciate it if you could help me get started. Can you tell me where to start contributing? Any pointers or links to external references would be really helpful. thank you for your time.

fruffy commented 1 month ago

Hello ,

I would like to take this up as my very first contribution to get into open source. I've never done this before and would really appreciate it if you could help me get started. Can you tell me where to start contributing? Any pointers or links to external references would be really helpful. thank you for your time.

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.

jafingerhut commented 1 month ago

@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.

antoninbas commented 1 month ago

@jafingerhut I suspect someone deleted the comment as it is unrelated to the issue

OsamaRab3 commented 1 month ago

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.

fruffy commented 1 month ago

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.