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

Added static asserts to fixed length header structures. #1693

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

Dimi1010
Copy link
Collaborator

The PR adds cpp11 static_assert statements after the declarations of fixed length structured used throughout Packet++ for encoding/decoding raw data.

Copy link

codecov bot commented Jan 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.11%. Comparing base (d60c699) to head (0d6c2c9).

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #1693      +/-   ##
==========================================
- Coverage   83.11%   83.11%   -0.01%     
==========================================
  Files         277      277              
  Lines       48207    48207              
  Branches    10192     9932     -260     
==========================================
- Hits        40069    40068       -1     
  Misses       7243     7243              
- Partials      895      896       +1     
Flag Coverage Δ
alpine320 75.11% <0.00%> (+<0.01%) ⬆️
fedora40 75.15% <100.00%> (-0.01%) ⬇️
macos-13 80.61% <100.00%> (-0.01%) ⬇️
macos-14 80.61% <100.00%> (ø)
macos-15 80.59% <100.00%> (ø)
mingw32 70.82% <0.00%> (-0.04%) ⬇️
mingw64 70.79% <0.00%> (-0.04%) ⬇️
npcap 85.24% <100.00%> (-0.02%) ⬇️
rhel94 75.01% <100.00%> (+0.03%) ⬆️
ubuntu2004 58.56% <ø> (-0.03%) ⬇️
ubuntu2004-zstd 58.68% <ø> (+<0.01%) ⬆️
ubuntu2204 74.90% <100.00%> (-0.01%) ⬇️
ubuntu2204-icpx 61.49% <100.00%> (+0.04%) ⬆️
ubuntu2404 75.15% <100.00%> (-0.01%) ⬇️
unittest 83.11% <100.00%> (-0.01%) ⬇️
windows-2019 85.27% <100.00%> (-0.01%) ⬇️
windows-2022 85.30% <100.00%> (-0.01%) ⬇️
winpcap 85.26% <100.00%> (ø)
xdp 50.53% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Dimi1010 Dimi1010 marked this pull request as ready for review January 19, 2025 19:02
@Dimi1010 Dimi1010 requested a review from seladb as a code owner January 19, 2025 19:02
@seladb
Copy link
Owner

seladb commented Jan 23, 2025

@Dimi1010 why are these static asserts needed? 🤔

@Dimi1010
Copy link
Collaborator Author

Dimi1010 commented Jan 23, 2025

@Dimi1010 why are these static asserts needed? 🤔

They aren't needed per se, but they provide compile time safety that the header structures are precisely the specified fixed length, as they will raise an error at compile time if not.

@seladb
Copy link
Owner

seladb commented Jan 23, 2025

@Dimi1010 why are these static asserts needed? 🤔

They aren't needed per se, but they provide compile time safety that the header structures are precisely the specified fixed length, as they will raise an error at compile time if not.

I always wondered why static asserts are needed 🙂
If someone decides to change a struct, it's probably for a good reason and they'll need to update the static assert as well.
Is there any real purpose for it?

@Dimi1010
Copy link
Collaborator Author

Dimi1010 commented Jan 23, 2025

I always wondered why static asserts are needed 🙂
If someone decides to change a struct, it's probably for a good reason and they'll need to update the static assert as well.
Is there any real purpose for it?

That is kinda the point. If you are changing the structs, it stops you by asking the question: "Do you really have a good reason to change it?". If you do, you change the assert too (and all the code that depends on the assert, hopefully).

My opinion in general about them is that it prevents accidental changes and also ensures that certain assumptions that the developers make about the code are correct, instead of only relying on the dev(and all other devs that work the same code) that the assumption holds.

It can also be used as a self documenting code. Without the assert, a person might not know that this struct is used in a way that needs it to be precisely N bytes long, unless they look through the code. There is the option of it being noted in documentation comments, but that is easier to forget to update than a static assert if making a change to the assumption.

@tigercosmos
Copy link
Collaborator

I think this change is helpful. It prevents people changing the struct layout accidently. I think asssertion and test are always useful.

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

Successfully merging this pull request may close these issues.

3 participants