Introduce default format for code base. #19

ViToni · 2017-05-05T23:05:50Z

The first commit introduces a new goal: make reformat based on clang-reformat (at least version 3.9 should be used).
This target is optional.

The second commit contains all changes after the execution of this command.

In the future committers can normalize their code by simply using the make goal.

pali · 2017-05-06T23:27:05Z

It is a good idea to have just one consistent coding style in whole project, but I dislike your proposed code style where are too many spaces around parenthesis. E.g. spaces around void in
static void debugQueue( void );

ViToni · 2017-05-07T00:10:24Z

Actually I don't mind which formatting style is to be used.
For me it's only important that there is one and that formatting is consistent all over the code base.

There are even some defaults styles (LLVM, Google, Chromium, Mozilla, WebKit) which could used:
http://releases.llvm.org/3.9.0/tools/clang/docs/ClangFormatStyleOptions.html

Just choose a style and we can stick to it.

pali · 2017-05-07T08:13:34Z

Try something like this?

BasedOnStyle: LLVM
Language: Cpp
IndentWidth: 4
ColumnLimit: 120
AllowShortFunctionsOnASingleLine: Empty
AlwaysBreakBeforeMultilineStrings: true

ViToni · 2017-05-07T08:30:20Z

AFAIK AllowShortFunctionsOnASingleLine should be either true or false.

I would suggest having UseTab: Never as one of the mandatory options as the mixture of tabs and whitespaces really messes diffs up.

Having also MaxEmptyLinesToKeep: 1 or MaxEmptyLinesToKeep: 2 might be useful, too.

ViToni · 2017-05-07T08:38:11Z

BTW I already pushed another version of reformatted source code settings (based on the initial format settings) when writting #19 (comment) , so you might want to check, if you like it more.

pali · 2017-05-07T08:55:03Z

AFAIK AllowShortFunctionsOnASingleLine should be either true or false

According to http://releases.llvm.org/3.9.0/tools/clang/docs/ClangFormatStyleOptions.html possible values are: None, Empty, Inline, All

I would suggest having UseTab: Never

Already part of BasedOnStyle: LLVM

Having also MaxEmptyLinesToKeep: 1

Also part of BasedOnStyle: LLVM

or MaxEmptyLinesToKeep: 2 might be useful, too.

BasedOnStyle: LLVM set it to 1, but 2 seems to be better value.

ViToni · 2017-05-07T12:13:31Z

AFAIK AllowShortFunctionsOnASingleLine should be either true or false

According to http://releases.llvm.org/3.9.0/tools/clang/docs/ClangFormatStyleOptions.html possible values are: None, Empty, Inline, All

You are right. (saw the boolean option here https://clangformat.com/)

I would suggest having UseTab: Never

Already part of BasedOnStyle: LLVM

It was more meant to be explicit.

I update the PR using these settings:

BasedOnStyle: LLVM
Language: Cpp

IndentWidth: 4
ContinuationIndentWidth: 8

ColumnLimit: 120

MaxEmptyLinesToKeep: 2

AllowShortFunctionsOnASingleLine: Empty
AlwaysBreakBeforeMultilineStrings: true

Signed-off-by: Victor Toni <[email protected]>

pali · 2017-05-07T19:40:26Z

src/ifvc.c

 }

 /*
-** Builds up a vector with the interface of the machine. Calls to the other functions of
+** Builds up a vector with the interface of the machine. Calls to the other
+*functions of


This is badly wrapped comment. Such changes are really not acceptable.

ViToni · 2017-05-07T22:54:29Z

Changed the mentioned part manually so that future calls of make reformat should work there nicely.

pali · 2017-05-07T23:08:59Z

src/confread.c

-            if(bufPtr == readSize) {
+        while (!finished) {
+            // If readpointer is at the end of the buffer, we should read next
+            // chunk...


pali · 2017-05-07T23:12:27Z

src/igmp.c

+            for (i = 0; i < MAX_UPS_VIFS; i++) {
+                if (-1 != upStreamIfIdx[i]) {
+                    // Check if the source address matches a valid address on upstream
+                    // vif.


pali · 2017-05-07T23:13:32Z

src/igmp.c

                        }
                    } else {
                        // Activate the route.
                        int vifindex = checkVIF->index;
-                        my_log(LOG_DEBUG, 0, "Route activate request from %s to %s on VIF[%d]",
-                            inetFmt(src,s1), inetFmt(dst,s2), vifindex);
+                        my_log(LOG_DEBUG, 0, "Route activate request from %s to %s on VIF[%d]", inetFmt(src, s1),


rather let inetFmt(src, s1) at second line after wrap

This is the result of automatic reformatting.

It can be changed either by having other formatting options (which would still look differently) or
it could be solved by using clang directives such as

int formatted_code; // clang-format off void unformatted_code ; // clang-format on void formatted_code_again;

I would prefer rather not introduce clang specific stuff into the code.

So what does it mean? It means that automatic reformatting is not a perfect solution and has problems.

Therefore we should use it only as help tool, not enforcement, like here.

pali · 2017-05-07T23:16:26Z

src/mcgroup.c

@@ -71,17 +65,19 @@ static int joinleave( int Cmd, int UdpSock, struct IfDesc *IfDp, uint32_t mcasta
 *   The join is bound to the UDP socket 'UdpSock', so if this socket is
 *   closed the membership is dropped.
 *
-*   @return 0 if the function succeeds, 1 if parameters are wrong or the join fails
+*   @return 0 if the function succeeds, 1 if parameters are wrong or the join
+* fails


* To make reformatting reproducible `make reformat` was used. Signed-off-by: Victor Toni <[email protected]>

pali · 2017-05-08T09:08:21Z

src/igmp.c

+                    } else if (src == checkVIF->InAdr.s_addr) {
+                        my_log(LOG_NOTICE, 0,
+                               "Route activation request from %s for %s is "
+                               "from myself. Ignoring.",


Please do not split string into more multiline parts.

pali · 2017-05-08T09:08:37Z

src/igmp.c

-                                inetFmt(src, s1), inetFmt(dst, s2), i);
+                            my_log(LOG_NOTICE, 0,
+                                   "The source address %s for group %s is "
+                                   "from downstream VIF[%d]. Ignoring.",


same here (and on other places)

pali · 2017-05-08T09:09:47Z

src/igmpproxy.c

+        "   -h   Display this help screen\n"
+        "   -d   Run in debug mode. Output all messages on stderr\n"
+        "   -v   Be verbose. Give twice to see even debug messages.\n"
+        "\n" PACKAGE_STRING "\n";


and here should be newline, after each "\n" to visually identify end of lines.

pali · 2017-05-08T09:10:23Z

src/igmpproxy.c


 // Global vars...
 static int sighandled = 0;
-#define GOT_SIGINT  0x01
-#define GOT_SIGHUP  0x02


let that space here, to have visually aligned those #defines

pali · 2017-05-08T09:11:39Z

src/igmpproxy.c

-            ) {
-                my_log( LOG_ERR, errno, "failed to detach daemon" );
+            if (close(0) < 0 || close(1) < 0 || close(2) < 0 || open("/dev/null", 0) != 0 || dup2(0, 1) < 0 ||
+                dup2(0, 2) < 0 || setpgid(0, 0) < 0) {


previous wrapping to 3 lines was better. close() calls on one line, open+dup() on second line and setpgid() on third.

pali · 2017-05-08T09:15:36Z

src/igmpproxy.h

+#define MAX_IP_PACKET_LEN 576
+#define MIN_IP_HEADER_LEN 20
+#define MAX_IP_HEADER_LEN 60
+#define IP_HEADER_RAOPT_LEN 24


And let those defines aligned as they were too

pali · 2017-05-08T09:16:56Z

src/lib.c

-             ((uint8_t *)&InAdr.s_addr)[ 0 ],
-             ((uint8_t *)&InAdr.s_addr)[ 1 ],
-             ((uint8_t *)&InAdr.s_addr)[ 2 ],
-             ((uint8_t *)&InAdr.s_addr)[ 3 ] );


And line wrapping here was also better before reformatting

pali · 2017-05-08T09:19:15Z

src/request.c

+     * queries.
+     *
+     * It might be better to send only a query on the interface the leave
+     * was accepted on and remove only that interface from the route.


This is not good reformat. Second sentence is also part of FIXME, but after change is not visually anymore.

pali · 2017-05-08T09:20:39Z

src/request.c

-                    inetFmt(Dp->InAdr.s_addr,s1),
-                    inetFmt(allhosts_group,s2),
-                    conf->queryResponseInterval);
+                my_log(LOG_DEBUG, 0, "Sent membership query from %s to %s. Delay: %d", inetFmt(Dp->InAdr.s_addr, s1),


here also put inetFmt on second line

pali · 2017-05-08T09:27:36Z

src/confread.c

+
+/*******************************************************************************
+ * Opens config file specified by filename.
+ ******************************************************************************/


This reformat (and on other places too) introduce inconsistency. As in other files is used just:

/** * ... * ... */

pali · 2017-10-30T15:16:22Z

@ViToni seems that auto-reformat by robot/machine for everything does not work as expected. After automatic reformat it needs some manual work to fix some parts. So I do not think that "make reformat" target is a good idea.

But, this is really an improvement for the code... so are you going to fix needed parts manually and finish this work in this pull request?

ViToni mentioned this pull request May 6, 2017

Improve logging #20

Open

ViToni force-pushed the feature/clang_format branch from 434a986 to b181619 Compare May 7, 2017 00:03

ViToni force-pushed the feature/clang_format branch from b181619 to 293bb10 Compare May 7, 2017 11:59

Add reformat target for consistent code formatting.

020c75c

Signed-off-by: Victor Toni <[email protected]>

ViToni force-pushed the feature/clang_format branch from 293bb10 to 4068ed9 Compare May 7, 2017 13:05

pali reviewed May 7, 2017

View reviewed changes

ViToni force-pushed the feature/clang_format branch from 4068ed9 to 5b47279 Compare May 7, 2017 22:51

pali reviewed May 7, 2017

View reviewed changes

ViToni force-pushed the feature/clang_format branch from 5b47279 to 60f7924 Compare May 7, 2017 23:15

pali reviewed May 7, 2017

View reviewed changes

ViToni force-pushed the feature/clang_format branch from 60f7924 to 0603abc Compare May 7, 2017 23:21

Reformat code base with clang-format-4.0.

dc5aa02

* To make reformatting reproducible `make reformat` was used. Signed-off-by: Victor Toni <[email protected]>

ViToni force-pushed the feature/clang_format branch from 0603abc to dc5aa02 Compare May 7, 2017 23:27

pali reviewed May 8, 2017

View reviewed changes

pali mentioned this pull request Aug 28, 2018

Feature/clean igmp #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce default format for code base. #19

Introduce default format for code base. #19

ViToni commented May 5, 2017

pali commented May 6, 2017

ViToni commented May 7, 2017

pali commented May 7, 2017

ViToni commented May 7, 2017 •

edited

Loading

ViToni commented May 7, 2017 •

edited

Loading

pali commented May 7, 2017

ViToni commented May 7, 2017 •

edited

Loading

pali May 7, 2017

ViToni commented May 7, 2017

pali May 7, 2017

pali May 7, 2017

pali May 7, 2017

ViToni May 7, 2017 •

edited

Loading

pali May 8, 2017

pali May 7, 2017

pali May 8, 2017

pali May 8, 2017

pali May 8, 2017

pali May 8, 2017

pali May 8, 2017

pali May 8, 2017

pali May 8, 2017

pali May 8, 2017

pali May 8, 2017

pali May 8, 2017

pali commented Oct 30, 2017

Introduce default format for code base. #19

Are you sure you want to change the base?

Introduce default format for code base. #19

Conversation

ViToni commented May 5, 2017

pali commented May 6, 2017

ViToni commented May 7, 2017

pali commented May 7, 2017

ViToni commented May 7, 2017 • edited Loading

ViToni commented May 7, 2017 • edited Loading

pali commented May 7, 2017

ViToni commented May 7, 2017 • edited Loading

Choose a reason for hiding this comment

ViToni commented May 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViToni May 7, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pali commented Oct 30, 2017

ViToni commented May 7, 2017 •

edited

Loading

ViToni commented May 7, 2017 •

edited

Loading

ViToni commented May 7, 2017 •

edited

Loading

ViToni May 7, 2017 •

edited

Loading