Clang-tidy fixes for Common++ #1639

egecetin · 2024-11-07T17:47:30Z

Fix various clang-tidy warnings for Common++

Requires #1677

codecov · 2024-11-07T18:17:01Z

Codecov Report

Attention: Patch coverage is 75.56818% with 43 lines in your changes missing coverage. Please review.

Project coverage is 83.11%. Comparing base (7898a5d) to head (a8779e0).

Files with missing lines	Patch %	Lines
Common++/src/Logger.cpp	16.66%	15 Missing ⚠️
Common++/src/SystemUtils.cpp	66.66%	7 Missing ⚠️
Common++/src/IpUtils.cpp	70.58%	3 Missing and 2 partials ⚠️
...ples/IcmpFileTransfer/IcmpFileTransfer-pitcher.cpp	0.00%	3 Missing ⚠️
Common++/src/IpAddress.cpp	93.54%	2 Missing ⚠️
Common++/src/OUILookup.cpp	77.77%	1 Missing and 1 partial ⚠️
Examples/ArpSpoofing/main.cpp	0.00%	2 Missing ⚠️
Common++/src/GeneralUtils.cpp	80.00%	1 Missing ⚠️
Common++/src/MacAddress.cpp	75.00%	0 Missing and 1 partial ⚠️
Examples/DnsSpoofing/main.cpp	0.00%	1 Missing ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##              dev    #1639      +/-   ##
==========================================
- Coverage   83.16%   83.11%   -0.05%     
==========================================
  Files         277      277              
  Lines       48201    48217      +16     
  Branches     9932     9957      +25     
==========================================
- Hits        40086    40077       -9     
+ Misses       7266     7264       -2     
- Partials      849      876      +27

Flag	Coverage Δ
alpine320	`75.11% <73.91%> (-0.05%)`	⬇️
fedora40	`75.16% <75.00%> (-0.01%)`	⬇️
macos-13	`80.62% <72.95%> (-0.04%)`	⬇️
macos-14	`80.62% <72.95%> (-0.04%)`	⬇️
macos-15	`80.60% <71.71%> (-0.04%)`	⬇️
mingw32	`70.82% <72.97%> (-0.09%)`	⬇️
mingw64	`70.79% <72.97%> (-0.09%)`	⬇️
npcap	`85.25% <81.35%> (-0.07%)`	⬇️
rhel94	`74.99% <75.00%> (-0.04%)`	⬇️
ubuntu2004	`58.57% <73.88%> (-0.06%)`	⬇️
ubuntu2004-zstd	`58.69% <73.88%> (-0.04%)`	⬇️
ubuntu2204	`74.91% <74.45%> (-0.07%)`	⬇️
ubuntu2204-icpx	`61.46% <67.88%> (+0.01%)`	⬆️
ubuntu2404	`75.16% <74.63%> (-0.07%)`	⬇️
unittest	`83.11% <75.56%> (-0.05%)`	⬇️
windows-2019	`85.28% <81.35%> (-0.07%)`	⬇️
windows-2022	`85.31% <81.35%> (-0.07%)`	⬇️
winpcap	`85.27% <81.35%> (-0.06%)`	⬇️
xdp	`50.52% <25.54%> (-0.02%)`	⬇️

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.

seladb · 2024-11-08T07:09:32Z

@egecetin please see this comment: #1637 (comment)

egecetin · 2024-11-08T07:55:26Z

@seladb Actually it is related with #1638 not this one 🙂. This PR for fixing of issues pointed by clang-tidy

seladb · 2024-11-08T08:02:48Z

@seladb Actually it is related with #1638 not this one 🙂. This PR for fixing of issues pointed by clang-tidy

Oops sorry, I commented on the wrong PR... 🤦‍♂️

Dimi1010 · 2024-11-14T21:07:33Z

.clang-format

@@ -21,4 +21,5 @@ BreakBeforeBraces: Custom
 BraceWrapping:
  SplitEmptyFunction: false
  AfterCaseLabel: true
+QualifierAlignment: Left


RIP, const on the right... I found that easier to read because its consistent behavior with its usage in the middle...

Let me check the codebase again. If most of them left it is better to split PR for changing it. Anyway, lets invite more people so we can decide which is better @seladb @tigercosmos @clementperon.

Note: We still have plenty of time because there are lots of warnings even for Common++ so it will take some time to fix PR :)

I like the left one though.

I just reverted it and placed all new qualifiers to left for now and keep the clang-format default behavior to keep this PR simple. We can completely change by just running clang-format later if left or right decided.

Copilot reviewed 3 out of 21 changed files in this pull request and generated no suggestions.

Files not reviewed (18)

Common++/header/GeneralUtils.h: Language not supported
Common++/header/IpAddress.h: Language not supported
Common++/header/IpUtils.h: Language not supported
Common++/header/LRUList.h: Language not supported
Common++/header/Logger.h: Language not supported
Common++/header/MacAddress.h: Language not supported
Common++/header/OUILookup.h: Language not supported
Common++/header/PointerVector.h: Language not supported
Common++/header/SystemUtils.h: Language not supported
Common++/header/TablePrinter.h: Language not supported
Common++/src/GeneralUtils.cpp: Language not supported
Common++/src/IpAddress.cpp: Language not supported
Common++/src/IpUtils.cpp: Language not supported
Common++/src/Logger.cpp: Language not supported
Common++/src/MacAddress.cpp: Language not supported
Common++/src/OUILookup.cpp: Language not supported
Common++/src/SystemUtils.cpp: Language not supported
Common++/src/TablePrinter.cpp: Language not supported

egecetin · 2025-01-01T18:29:56Z

.clang-tidy-new

@@ -0,0 +1,34 @@
+Checks: 'cert-*,


I used this configuration to fix issues. Since all of directories are not fixed this file not used for now in CI

egecetin · 2025-01-01T18:32:28Z

ci/clang-tidy-all-new.sh

@@ -0,0 +1,43 @@
+#!/bin/sh


This script for easy local checking. Except Common++ all directories are ignored. It will be replacing the original script after all folders are fixed.

egecetin · 2025-01-01T18:57:45Z

Side note looks like gcovr weirdly fails in Fedora 40. Check https://github.com/seladb/PcapPlusPlus/actions/runs/12573299593/job/35046174314. I only added a newline in commit 12e7606 and it passed. It might affect other PRs too please keep in mind that if you see similar error.

seladb · 2025-01-03T06:52:36Z

Common++/header/IpAddress.h

+		uint32_t m_NetworkPrefix{};
+		uint32_t m_Mask{};


Why not set it to zero instead of {}? 🤔

It is from clang-tidy auto-fix. I don't touch anything as much as possible unless something is broken (unfortunately it messed with OUILookup but fixed) or suggested fixes not applied automatically. {} should be zero for fundamental types if I'm not mistaken

{} should be zero for fundamental types if I'm not mistaken

I believe you're right, but it makes the code less clear. Is there a configuration to ask clang-tidy auto-fix to set it to zero instead?

seladb · 2025-01-03T06:53:07Z

Common++/header/IpAddress.h

+		uint8_t m_NetworkPrefix[16]{};
+		uint8_t m_Mask[16]{};


Common++/header/Logger.h

Common++/src/Logger.cpp

Common++/src/SystemUtils.cpp

seladb · 2025-01-03T07:08:35Z

Common++/src/SystemUtils.cpp

+		struct sigaction action
+		{
+		};


Common++/src/TablePrinter.cpp

Common++/header/SystemUtils.h

Common++/src/IpAddress.cpp

seladb · 2025-01-08T03:32:25Z

Pcap++/src/PcapLiveDevice.cpp

@@ -249,7 +249,7 @@ namespace pcpp
 			PcapStats stats;
 			getStatistics(stats);
 			m_cbOnStatsUpdate(stats, m_cbOnStatsUpdateUserCookie);
-			multiPlatformSleep(m_IntervalToUpdateStats);
+			std::this_thread::sleep_for(std::chrono::milliseconds(m_IntervalToUpdateStats));


This should be:

std::this_thread::sleep_for(std::chrono::seconds(m_IntervalToUpdateStats));

seladb · 2025-01-08T03:33:46Z

Common++/src/SystemUtils.cpp

+		struct stat info
+		{
+		};


I see you already opened a PR for it: #1677, let's get it merged before this PR?

seladb · 2025-01-08T07:37:44Z

Common++/src/IpAddress.cpp

@@ -364,15 +367,15 @@ namespace pcpp
 	{
 		memset(m_Mask, 0, IPV6_ADDR_SIZE);
 		int remainingPrefixLen = prefixLen;
-		for (auto byteIndex = 0; byteIndex < IPV6_ADDR_SIZE; byteIndex++)
+		for (unsigned char& byteIndex : m_Mask)


We can rename byteIndex to byte and use auto&:

for (auto& byte : m_Mask) { ... }

seladb · 2025-01-08T07:38:17Z

Common++/src/IpAddress.cpp

@@ -484,10 +488,10 @@ namespace pcpp
 	uint8_t IPv6Network::getPrefixLen() const
 	{
 		uint8_t result = 0;
-		for (auto byteIndex = 0; byteIndex < IPV6_ADDR_SIZE; byteIndex++)
+		for (const unsigned char byteIndex : m_Mask)


We can rename byteIndex to byte and use auto:

for (auto byte : m_Mask) { ... }

seladb · 2025-01-08T07:38:38Z

Common++/src/IpAddress.cpp

@@ -520,10 +524,10 @@ namespace pcpp
 	uint64_t IPv6Network::getTotalAddressCount() const
 	{
 		int numOfBitset = 0;
-		for (auto byteIndex = 0; byteIndex < IPV6_ADDR_SIZE; byteIndex++)
+		for (const unsigned char byteIndex : m_Mask)


egecetin force-pushed the clang-tidy branch from 559ae1d to 7e9c12f Compare November 9, 2024 08:04

egecetin changed the title ~~Clang-tidy fixes~~ Clang-tidy fixes for Common++ Nov 9, 2024

egecetin force-pushed the clang-tidy branch from 7e9c12f to c4cac45 Compare November 9, 2024 10:40

Dimi1010 reviewed Nov 14, 2024

View reviewed changes

egecetin requested a review from Copilot November 19, 2024 19:57

Copilot AI reviewed Nov 19, 2024

View reviewed changes

egecetin force-pushed the clang-tidy branch from c4561c0 to cd29b7a Compare December 25, 2024 18:50

apply fixes for Common++

fbafcd8

egecetin force-pushed the clang-tidy branch from a94fdc6 to fbafcd8 Compare January 1, 2025 13:53

egecetin added 8 commits January 1, 2025 16:53

Merge remote-tracking branch 'upstream/dev' into clang-tidy

f890e2c

minor fixes

4fc7a1c

fix last issues

b5c720e

define a thread safe strerror

5727149

include array

c645a6c

format

da67602

not use scripts until all repo updated

2da1d0b

revert unnecessary change

53b7080

egecetin mentioned this pull request Jan 1, 2025

Enable more clang-tidy checks #1675

Open

6 tasks

egecetin marked this pull request as ready for review January 1, 2025 18:23

egecetin requested a review from seladb as a code owner January 1, 2025 18:23

mark executable

f951e47

egecetin commented Jan 1, 2025

View reviewed changes

add newline

12e7606

seladb reviewed Jan 3, 2025

View reviewed changes

egecetin and others added 10 commits January 3, 2025 21:16

rename getErrorString

b02481f

deprecate multiplatformsleep

8270a39

format

a245c83

fix some weird struct braces

b4ad4a5

replace all multiplatformsleep calls

cd15bc7

include thread for dpdk tests

59b14fb

include thread lib

8df0870

format

3b55f05

fix braces for clang-format 19+

70a5233

Merge branch 'dev' into clang-tidy

a8779e0

seladb reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clang-tidy fixes for Common++ #1639

Clang-tidy fixes for Common++ #1639

egecetin commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

seladb commented Nov 8, 2024

egecetin commented Nov 8, 2024 •

edited

Loading

seladb commented Nov 8, 2024

Dimi1010 Nov 14, 2024

egecetin Dec 1, 2024

tigercosmos Dec 4, 2024

egecetin Jan 1, 2025

egecetin Jan 1, 2025

egecetin Jan 1, 2025 •

edited

Loading

egecetin commented Jan 1, 2025

seladb Jan 3, 2025

egecetin Jan 3, 2025

seladb Jan 8, 2025 •

edited

Loading

seladb Jan 3, 2025

seladb Jan 3, 2025

seladb Jan 8, 2025

seladb Jan 8, 2025

seladb Jan 8, 2025

seladb Jan 8, 2025

seladb Jan 8, 2025

Clang-tidy fixes for Common++ #1639

Are you sure you want to change the base?

Clang-tidy fixes for Common++ #1639

Conversation

egecetin commented Nov 7, 2024 • edited Loading

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

seladb commented Nov 8, 2024

egecetin commented Nov 8, 2024 • edited Loading

seladb commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egecetin Jan 1, 2025 • edited Loading

Choose a reason for hiding this comment

egecetin commented Jan 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seladb Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

egecetin commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

egecetin commented Nov 8, 2024 •

edited

Loading

egecetin Jan 1, 2025 •

edited

Loading

seladb Jan 8, 2025 •

edited

Loading