PcapPlusPlus icon indicating copy to clipboard operation
PcapPlusPlus copied to clipboard

Port the code to C++11

Open seladb opened this issue 3 years ago • 34 comments
trafficstars

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

seladb avatar Oct 08 '22 09:10 seladb

I'd like to help with this.

InfiniteVerma avatar Oct 11 '22 18:10 InfiniteVerma

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.

egecetin avatar Jan 10 '23 10:01 egecetin

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

jpcofr avatar Sep 05 '23 17:09 jpcofr

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! 🙏 🙏

seladb avatar Sep 06 '23 05:09 seladb

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 avatar Sep 06 '23 05:09 jpcofr

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

seladb avatar Sep 06 '23 07:09 seladb

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

tigercosmos avatar Nov 30 '23 18:11 tigercosmos

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

seladb avatar Dec 01 '23 07:12 seladb

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

Dimi1010 avatar Feb 26 '24 10:02 Dimi1010

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

rtmeng avatar Apr 03 '24 20:04 rtmeng

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

seladb avatar Apr 03 '24 20:04 seladb

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?

rtmeng avatar Apr 03 '24 21:04 rtmeng

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.

tigercosmos avatar Apr 04 '24 00:04 tigercosmos

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

seladb avatar Apr 04 '24 06:04 seladb

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

tigercosmos avatar Jun 05 '24 06:06 tigercosmos

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

Dimi1010 avatar Jun 05 '24 08:06 Dimi1010

@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

egecetin avatar Jun 05 '24 10:06 egecetin