Thread (34 messages) 34 messages, 6 authors, 2017-09-28

Re: [PATCH v2 1/2] clang-format: outline the git project's coding style

flat view

From: Brandon Williams <hidden>
Date: 2017-08-15 17:37:43

On 08/15, Ben Peart wrote:
quoted hunk ↗ jump to hunk

On 8/14/2017 6:02 PM, Stefan Beller wrote:
quoted
On Mon, Aug 14, 2017 at 2:30 PM, Brandon Williams [off-list ref] wrote:
quoted
Add a '.clang-format' file which outlines the git project's coding
style.  This can be used with clang-format to auto-format .c and .h
files to conform with git's style.

Signed-off-by: Brandon Williams <redacted>
Applying this patch and running
    clang-format -i -style file *.c *.h builtin/*.c
produces a diff, that I'd mostly agree with.
This style guide is close to our current style.
I'm happy to see progress being made in helping reduce the time
spent manually reviewing and fixing style formatting errors.  In an
effort to help, I installed this in Windows and tried it as well.
The tools all appear to be working fine and are supported on
Windows.

For the most part, the formatting rules look pretty consistent with
the existing style.  I ran the same test and looked at the diffs and
saw a couple of things that looked odd. For example, how it wrapped
the "static int" on the function header below was different.  Not
sure why as it didn't wrap all the other function headers the same
even later in the file it didn't do that with "static void
mute_routine"
diff --git a/apply.c b/apply.c
index f2d599141d..bb77242e3d 100644
--- a/apply.c
+++ b/apply.c
@@ -58,12 +59,11 @@ static int parse_whitespace_option(struct
apply_state *state, const char *option
        return error(_("unrecognized whitespace option '%s'"), option);
 }

-static int parse_ignorewhitespace_option(struct apply_state *state,
-                                                const char *option)
+static int
+parse_ignorewhitespace_option(struct apply_state *state, const char
*option)
 {
-       if (!option || !strcmp(option, "no") ||
-           !strcmp(option, "false") || !strcmp(option, "never") ||
-           !strcmp(option, "none")) {
+       if (!option || !strcmp(option, "no") || !strcmp(option, "false") ||
+           !strcmp(option, "never") || !strcmp(option, "none")) {
                state->ws_ignore_action = ignore_ws_none;
                return 0;
        }


Later in the file it wraps some of them again: (add_line_info,
prepare_image, find_name_common, etc).  Again, it appears to be
inconsistent but there must be some rule that is causing this
behavior.
These have to deal with setting the penalties.  When a line gets to be
too long the tool needs to find a place to break the line based on a
penalty system.  The current .clang-format file I sent out has values
for the penalties which would most likely need to be tweaked through
trial and error.
quoted hunk ↗ jump to hunk


Here is an example of how it wrapped bit fields differently.  Again,
it didn't seem to be consistent with itself as just below this, it
left them on separate lines.

@@ -182,8 +185,7 @@ struct fragment {
         * but some codepaths store an allocated buffer.
         */
        const char *patch;
-       unsigned free_patch:1,
-               rejected:1;
+       unsigned free_patch : 1, rejected : 1;
        int size;
        int linenr;
        struct fragment *next;
If the return type was replicated then it would probably format the
different struct members on their own line.

Big thanks to those working on this!
quoted
As noted in patch 2/2 we'd now need an easy way to
expose this for use in various situations, such as
* contributor wanting to format their patch
* reformatting code for readability

Thanks,
Stefan
-- 
Brandon Williams
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help