PcapPlusPlus
PcapPlusPlus copied to clipboard
Port the code to C++11
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
nullptrinstead ofNULL~~ - Use
autowhere possible - Use
enum classmore - Use range-based loops where possible
- Use
delete,default,overridewhere needed - Use
unordered_mapinstead ofmapfor 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
I'd like to help with this.
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.
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 @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! 🙏 🙏
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...
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
btw, maybe we can change the issue to C++20 directly.
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
Another thing is replacing ostringstream usages with to_string where it is used to convert only a single value to string.
Hi @seladb , I would also like to collaborate on this, is it still open?
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
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?
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.
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
@Dimi1010 What are your thoughts on how much we still have to finish?
@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_ptrandshared_ptrover the raw pointers for dynamic memory management. - Increased preference of "pass-by-value +
std::move" instead ofconst 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.
@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