Clang format for `Common++` #1451

tigercosmos · 2024-06-17T07:48:15Z

apply clang-format for Common++

tigercosmos · 2024-06-18T08:51:38Z

.clang-format

 AllowShortFunctionsOnASingleLine: All
 NamespaceIndentation: All
-
+SortIncludes: false


prevent some possible includes' error due to the ordering.

tigercosmos · 2024-06-18T08:52:01Z

.clang-format

@@ -1,7 +1,8 @@
 ---
 BasedOnStyle: Microsoft
 UseTab: Always
+BreakBeforeBraces: Allman


the style like this:

void Foo () { // ... }

tigercosmos · 2024-06-18T08:52:14Z

.github/workflows/build_and_test.yml

@@ -30,6 +30,7 @@ jobs:
        run: |
          apk update && apk add cppcheck python3-dev
          python3 -m pip install cmake-format
+          pip install clang-format==18.1.6


specify the clang-formant here.

tigercosmos · 2024-06-18T08:52:24Z

.pre-commit-config.yaml

@@ -16,9 +16,11 @@ repos:
  - repo: https://github.com/pocc/pre-commit-hooks
    rev: v1.3.5
    hooks:
+      - id: clang-format
+        args: ["--style=file"]  # Use the .clang-format file for configuration
+        files: ^Common\+\+/.*\.(cpp|h)$


apply on Common++ for now.

tigercosmos · 2024-06-18T08:53:13Z

Common++/header/DeprecationUtils.h

-        #pragma message("WARNING: Disabling of warnings is not implemented for this compiler")
-        #define DISABLE_WARNING_PUSH
-        #define DISABLE_WARNING_POP
+// clang-format off


here is the manual fix, since the clang-formant cannot do the formating well here. it tries to separate -Wdeprecated-declarations to -Wdeprecated - declarations

seladb · 2024-06-18T09:18:19Z

@tigercosmos the PR title should be:
"Clang format for Common++", not Commit++ 🙂

tigercosmos · 2024-06-18T09:23:26Z

sorry, fixed the title.

btw, seems that the CI already runs the check for clang-format

This reverts commit 8531104.

tigercosmos · 2024-06-18T09:26:38Z

Sorry, I think it's okay to squash the commits, and we can add the ignore hash in the next PR.

Common++/header/DeprecationUtils.h

tigercosmos · 2024-06-19T01:26:27Z

.clang-format

 AllowShortFunctionsOnASingleLine: All
 NamespaceIndentation: All
-
+SortIncludes: false
+IndentPPDirectives: AfterHash


let the macro has indent

tigercosmos · 2024-06-19T01:31:32Z

.clang-format

-
+SortIncludes: false
+IndentPPDirectives: AfterHash
+IndentWidth: 4


macro indent width

Common++/header/IpAddressUtils.h

Common++/header/GeneralUtils.h

Common++/header/IpAddress.h

Common++/header/Logger.h

seladb · 2024-06-19T07:08:52Z

Common++/src/Logger.cpp

+		sstream << file << ": " << method << ":" << line;
+		std::cout << std::left << "[" << std::setw(5) << Logger::logLevelAsString(logLevel) << ": " << std::setw(45)
+				  << sstream.str() << "] " << logMessage << std::endl;


I actually like the previous version better, but not sure if there is a way to keep it...

I cannot find the solution for this.

Common++/src/SystemUtils.cpp

seladb · 2024-06-20T07:33:41Z

Common++/header/IpAddress.h

+		IPv4Address(const std::array<uint8_t, 4>& bytes) : m_Bytes(bytes)
+		{
+		}


This formatting change doesn't make a lot of sense, is there an option to avoid it?

this one is due to BreakBeforeBraces: Allman

If we change to Custom we can set SplitEmptyFunction: false which I think should solve it:
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#bracewrapping

actually, I would stand for splitting the braces in this case. shall we just use the default Allman settings to prevent too much customization from using Custom?

I don't know the default values of the different flags under BraceWrapping. If all the defaults are what we need, I think it's ok to set:

BreakBeforeBraces: Custom BraceWrapping: SplitEmptyFunction: false

I tried your config, and the effect is like this:

void matchNetwork(const std::string& network) {}

become this:

void matchNetwork(const std::string& network) {}

this is slightly better I think. The rest of the curly braces formatting stays the same?

if there is something in the braces, yes, it will be the same.

void matchNetwork(const std::string& network) { // ... }

sure, applied your config.

Common++/header/LRUList.h

seladb · 2024-06-20T07:44:19Z

Common++/header/MacAddress.h

-		MacAddress(const T& addr) : MacAddress(static_cast<std::string>(addr)) {};
-
+		template <typename T, typename = typename std::enable_if<std::is_convertible<T, std::string>::value>::type>
+		MacAddress(const T& addr) : MacAddress(static_cast<std::string>(addr)){};


Here it keeps the empty curly braces {} in the same line, as opposed to IpAddress.h where it didn't. Any idea why?

maybe because it's a template?

I think I know why, probably due to ;

yeah, after I removed ;, it becomes normal.
But for now I can do manually fixing, but after when we apply a lot of files, it's hard to pick these syntax issues one by one.

oh, maybe we should just remove the ;

yeah, after I removed ;, it becomes normal. But for now I can do manually fixing, but after when we apply a lot of files, it's hard to pick these syntax issues one by one.

we should fix things whenever we see them, of course we might miss some...

seladb · 2024-06-20T07:56:40Z

Common++/src/OUILookup.cpp

+			vendorMap.insert({
+			    std::stoull(line.key()), { val["vendor"], vLocalMaskedFilter }
+            });


There is a weird mix of tabs and spaces in these lines, but not sure if it can be fixed 🤔

it is in purpose. the indent is tab, the alignment is space.
for example:

<tab> void foo(int a, <tab><spaces> int b) {}

yeah, but why does line 57 has spaces instead of tabs? Maybe we should just move }); to line 56?

yeah... you are right.. it's weird here as well.

I tried all options of UseTab, and all options decide to use spaces there
https://clang.llvm.org/docs/ClangFormatStyleOptions.html#usetab

Seems it is something that we cannot control, sometime clang-tidy is stupid.

did you try moving }); from line 57 to line 56 before running clang-format?

I tried, it doesn't help
no matter this;

vendorMap.insert({std::stoull(line.key()), { val["vendor"], vLocalMaskedFilter }});

or

vendorMap.insert( {std::stoull(line.key()), { val["vendor"], vLocalMaskedFilter }});

they will be formatted as:

vendorMap.insert({ std::stoull(line.key()), { val["vendor"], vLocalMaskedFilter } });

one way is to use // clang-format off here, but I would suggest just let it be this if it doesn't break the build.
according to my experience, when there is too many braces, clang-format becomes stupid.

yes, we can just leave it the way it is... 😕

Common++/src/SystemUtils.cpp

seladb · 2024-06-20T08:00:34Z

. git-blame-ignore-revs

Should we add this as empty file in this commit, or add it later after we merge?

I think we can have it now, we will add the file anyway.

I don't have a strong opinion, but I think it's better to add the file after we have the commit hash. But if you feel strongly about keeping it, I'm ok with it

tigercosmos requested a review from seladb as a code owner June 17, 2024 07:48

tigercosmos marked this pull request as draft June 17, 2024 07:48

tigercosmos added this to the Augest 2024 Release milestone Jun 17, 2024

tigercosmos force-pushed the clang-format branch from 105ba35 to c961720 Compare June 18, 2024 08:50

tigercosmos commented Jun 18, 2024

View reviewed changes

add clang-format in CI

884bcf7

tigercosmos changed the title ~~Clang format~~ Clang format for Commit++ Jun 18, 2024

apply clang-format in Common++

3b0b228

tigercosmos force-pushed the clang-format branch from 2aca568 to 9c7986b Compare June 18, 2024 09:06

tigercosmos changed the base branch from master to dev June 18, 2024 09:07

tigercosmos marked this pull request as ready for review June 18, 2024 09:07

ignore format commit

8531104

tigercosmos force-pushed the clang-format branch from 9c7986b to 8531104 Compare June 18, 2024 09:08

concate install

e30c338

tigercosmos changed the title ~~Clang format for Commit++~~ Clang format for Common++ Jun 18, 2024

tigercosmos and others added 2 commits June 18, 2024 18:25

Revert "ignore format commit"

bddf121

This reverts commit 8531104.

Merge branch 'dev' into clang-format

beba299

Merge branch 'clang-format-ci' into clang-format

47644ee

Dimi1010 reviewed Jun 18, 2024

View reviewed changes

Common++/header/DeprecationUtils.h Outdated Show resolved Hide resolved

add indent for macro

7a8d3ea

tigercosmos commented Jun 19, 2024

View reviewed changes

.clang-format

SortIncludes: false

IndentPPDirectives: AfterHash

IndentWidth: 4

Copy link

Collaborator Author

tigercosmos Jun 19, 2024

Choose a reason for hiding this comment

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

macro indent width

tigercosmos mentioned this pull request Jun 19, 2024

add clang-format in CI #1453

Closed

seladb reviewed Jun 19, 2024

View reviewed changes

tigercosmos added 3 commits June 19, 2024 17:55

update

593f0b4

rerun the clang-format

2bb033a

tune

1f3ef6b

Cpp11BracedListStyle: false

7c369d0

tigercosmos requested a review from seladb June 19, 2024 09:06

seladb reviewed Jun 20, 2024

View reviewed changes

tigercosmos added 3 commits June 20, 2024 17:20

fix ;

8705347

change to use AlignWithSpaces

f79b93e

BreakBeforeBraces: Custom

9da0f91

tigercosmos requested a review from seladb June 20, 2024 09:52

seladb approved these changes Jun 21, 2024

View reviewed changes

tigercosmos merged commit e3128c6 into seladb:dev Jun 21, 2024
39 checks passed

tigercosmos deleted the clang-format branch June 21, 2024 07:24

Clang format for Common++ #1451

Clang format for Common++ #1451

Conversation

tigercosmos commented Jun 17, 2024 • 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

seladb commented Jun 18, 2024

tigercosmos commented Jun 18, 2024

tigercosmos commented Jun 18, 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

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

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

tigercosmos Jun 20, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Clang format for `Common++` #1451

Clang format for `Common++` #1451

tigercosmos commented Jun 17, 2024 •

edited

Loading

tigercosmos Jun 20, 2024 •

edited

Loading