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

Port the code to C++11 #977

Open
seladb opened this issue Oct 8, 2022 · 17 comments
Open

Port the code to C++11 #977

seladb opened this issue Oct 8, 2022 · 17 comments

Comments

@seladb
Copy link
Owner

seladb commented Oct 8, 2022

We recently moved to C++11 and big parts of the code are still using C++98 style.
This will be an ongoing task to port more and more parts of the code to C++11.

For example:

  • Use nullptr instead of NULL
  • Use auto where possible
  • Use enum class more
  • Use range-based loops where possible
  • Use delete, default, override where needed
  • Use unordered_map instead of map for better performance
  • etc.

IMPORTANT: this change can (and should) be done incrementally. Please try to open small PRs with not a lot of changes that are easy to review

@InfiniteVerma
Copy link

I'd like to help with this.

@egecetin
Copy link
Collaborator

Not directly related with C++11 but also, library have some many C style casts, which can be easily located by using cppcheck. Anyone who wants fix them just remove or comment cstyleCast:* line from cppcheckSuppressions.txt and run pre-commit run --all-files or manually run cppcheck from command line.

@jpcofr
Copy link
Contributor

jpcofr commented Sep 5, 2023

Hi @seladb - I would like to collaborate on this task. Is there any task breakdown already or is there any progress?
Also, why porting to C++11 instead of later versions? (is it because the latest port you mention in your first comment was to C++11?

UPDATE - based on the Release notes (changes from v22.05) it seems this issue/ticket should be closed...

@seladb
Copy link
Owner Author

seladb commented Sep 6, 2023

Hi @seladb - I would like to collaborate on this task. Is there any task breakdown already or is there any progress? Also, why porting to C++11 instead of later versions? (is it because the latest port you mention in your first comment was to C++11?

UPDATE - based on the Release notes (changes from v22.05) it seems this issue/ticket should be closed...

Hi @jpcofr , thanks for offering your help! Actually, the reason this issue is still open is because this is an ongoing effort. We migrated to C++11 about a year ago, but a lot of the code base is still in C++98 style (definitely most of the code that was written more than a year ago). For example: we don't always use auto where needed, we still use std::map where we could use std::unordered_map, we don't use range-based loops in many cases, etc. We could use your help with those.

As I mentioned above - we don't expect a huge PR that fixes all of these things. We actually prefer small PRs that make incremental fixes, and we hope that we can slowly migrate as much of the code as possible.

It'd be really great if you can help. I'd like to thank you again and I'm looking forward to seeing your contributions! 🙏 🙏

@jpcofr
Copy link
Contributor

jpcofr commented Sep 6, 2023

be really great if you can help.

@seladb ok I can collaborate. I am new to this repo so I need to check out how is everything working. What would be the next namespace/class etc. that you would like to be ported? I can start from there...

@seladb
Copy link
Owner Author

seladb commented Sep 6, 2023

be really great if you can help.

@seladb ok I can collaborate. I am new to this repo so I need to check out how is everything working. What would be the next namespace/class etc. that you would like to be ported? I can start from there...

@jpcofr If you want you can start with HttpAnalyzer. It's a quite simple utility (or an example of how to use PcapPlusPlus), and it uses a std::map that can be converted to std::unordered_map. It also has quite a few for loops that can be modified, and I'm sure we can use the auto keyword more.
This utility has python-based tests that you can run locally, and they also run in GitHub Actions as part of the CI: https://github.com/seladb/PcapPlusPlus/actions/runs/5988356929/job/16243457492#step:8:65

Please let me know if you need any help

@tigercosmos
Copy link
Collaborator

btw, maybe we can change the issue to C++20 directly.

@seladb
Copy link
Owner Author

seladb commented Dec 1, 2023

btw, maybe we can change the issue to C++20 directly.

@tigercosmos for now I prefer to keep supporting C++11 to support a broader variety of platforms and devices

@Dimi1010
Copy link
Collaborator

Dimi1010 commented Feb 26, 2024

Another thing is replacing ostringstream usages with to_string where it is used to convert only a single value to string.

@rtmeng
Copy link
Contributor

rtmeng commented Apr 3, 2024

Hi @seladb , I would also like to collaborate on this, is it still open?

@seladb
Copy link
Owner Author

seladb commented Apr 3, 2024

Hi @seladb , I would also like to collaborate on this, is it still open?

Yes sure, you're more than welcome to contribute! 🙏
Feel free to open PRs and we'll review

@rtmeng
Copy link
Contributor

rtmeng commented Apr 3, 2024

Hi @seladb , I would also like to collaborate on this, is it still open?

Yes sure, you're more than welcome to contribute! 🙏 Feel free to open PRs and we'll review

Great! I'm still looking through the repo but is there anywhere I should start for what should be ported next?

@tigercosmos
Copy link
Collaborator

Hi @seladb , I would also like to collaborate on this, is it still open?

Yes sure, you're more than welcome to contribute! 🙏 Feel free to open PRs and we'll review

Great! I'm still looking through the repo but is there anywhere I should start for what should be ported next?

No, you can start at any place that uses C++98 instead of C++11. You can also refer to some merged PR for references.

@seladb
Copy link
Owner Author

seladb commented Apr 4, 2024

Hi @rtmeng ! I just replied to your email. If you're interested in something nice and simple to get into the project, you can try modernizing the tutorials code

@tigercosmos
Copy link
Collaborator

@Dimi1010 What are your thoughts on how much we still have to finish?

@Dimi1010
Copy link
Collaborator

Dimi1010 commented Jun 5, 2024

@Dimi1010 What are your thoughts on how much we still have to finish?

@tigercosmos

I think the map -> unordered_map is almost done. The only place I can find a #include <map> is in TcpReassembly.h and TextBasedProtocol.h, tho I haven't checked if that is deliberate.

For C-casts, I clean them up as I find them, but there are still plenty to go around... and it sucks you can't find them easily with a find command...

Two things I might add to the list:

  • Increased preference on using unique_ptr and shared_ptr over the raw pointers for dynamic memory management.
  • Increased preference of "pass-by-value + std::move" instead of const T& for constructor parameters that are just copied into member fields, as that would allow moving the parameter into the member field via double move than copying even when its not strictly required. Reasoning: https://stackoverflow.com/a/51706522/11922936
    • Setup of perfect forwarding is also an option, but its more of a hassle and requires templates.

@egecetin
Copy link
Collaborator

egecetin commented Jun 5, 2024

@Dimi1010 For c style casts you can use cppcheck. It should point most of them. Just remove cstyleCast:* line from cppcheckSuppressions.txt and run pre-commit locally or directly cppcheck. It is up to you which one you run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants