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

Run clang-format for Examples and Tutorials #1464

Draft
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

seladb
Copy link
Owner

@seladb seladb commented Jun 24, 2024

No description provided.

Copy link

codecov bot commented Jun 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.68%. Comparing base (959369d) to head (fe1dfb0).

Additional details and impacted files
@@           Coverage Diff           @@
##              dev    #1464   +/-   ##
=======================================
  Coverage   82.67%   82.68%           
=======================================
  Files         171      171           
  Lines       22623    22623           
  Branches     8570     8570           
=======================================
+ Hits        18704    18705    +1     
+ Misses       3103     3093   -10     
- Partials      816      825    +9     
Flag Coverage Δ
alpine317 72.73% <ø> (+<0.01%) ⬆️
fedora37 72.69% <ø> (-0.05%) ⬇️
macos-12 61.50% <ø> (ø)
macos-13 60.52% <ø> (ø)
macos-14 60.52% <ø> (ø)
mingw32 70.69% <ø> (ø)
mingw64 70.69% <ø> (ø)
npcap 83.96% <ø> (ø)
rhel93 72.75% <ø> (-0.01%) ⬇️
ubuntu1804 75.26% <ø> (+0.04%) ⬆️
ubuntu2004 73.70% <ø> (+0.04%) ⬆️
ubuntu2004-zstd 73.70% <ø> (-0.01%) ⬇️
ubuntu2204 72.56% <ø> (ø)
ubuntu2204-icpx 59.03% <ø> (ø)
unittest 82.68% <ø> (+<0.01%) ⬆️
windows-2019 83.97% <ø> (ø)
windows-2022 83.98% <ø> (ø)
winpcap 83.94% <ø> (-0.01%) ⬇️
xdp 60.19% <ø> (ø)

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.

Comment on lines 26 to 31
{ "interface", required_argument, nullptr, 'i' },
{ "victim", required_argument, nullptr, 'c' },
{ "gateway", required_argument, nullptr, 'g' },
{ "help", no_argument, nullptr, 'h' },
{ "version", no_argument, nullptr, 'v' },
{ nullptr, 0, nullptr, 0 }
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tigercosmos for some reason clang-format mixes tabs and spaces here, I have no idea why 🤷‍♂️
I tried different things but couldn't make it to work. Maybe you know?

It happens in other files also, like here:

static struct option IcmpFTOptions[] = {

But in others it doesn't happen:

static struct option DNSResolverOptions[] = {

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently in some of the files, changing the order of the arguments fixes the issue, this is very weird 😕

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seladb whenever you think clang-format is stupid, you can turn off the format for that part.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's concerning that we can't rely on clang-format and need to go over the code ourselves. This kind of beats the purpose of a code formatter 😕

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@seladb this kind of stuff rarely happens. We are formatting a bunch of code at once so there might be a few weird formatting. But overall, we will get the fruits after all formatting. :)

.clang-format Outdated
@@ -1,6 +1,6 @@
---
BasedOnStyle: Microsoft
UseTab: AlignWithSpaces
UseTab: ForIndentation
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tigercosmos I think we should change this, it looks nicer, what do you think?
If you agree I can open a PR for Common++ as well

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion about it. it's fine to me.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I'll open a separate PR for this change

@tigercosmos
Copy link
Collaborator

@seladb precommit failed. did you check if your clang-format version correct?

@seladb
Copy link
Owner Author

seladb commented Jun 24, 2024

@seladb precommit failed. did you check if your clang-format version correct?

Hmm I'm using version 18.1.6 on Windows

D:\seladb\PcapPlusPlus\Examples>D:\Apps\LLVM\bin\clang.exe --version
clang version 18.1.6
Target: x86_64-pc-windows-msvc
Thread model: posix

@tigercosmos
Copy link
Collaborator

@seladb remember to update .pre-commit.config file for the tested files.

@seladb
Copy link
Owner Author

seladb commented Jun 28, 2024

@seladb remember to update .pre-commit.config file for the tested files.

Yes, that's a good point, thanks for the reminder!

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.

None yet

2 participants