Clang format

[dougnazar]

Here's an initial clang-format style file. It uses the Google Style (found in the Coding Standards wiki) as the base but I increased the column width to 160 based on some of the lines in the existing code base.

The second commit is an example of letting clang-format re-write src/zm_stream.* (via clang-format -i src/zm_stream.*)

For those not familiar with clang-format, there is a built-in command in git, git clang-format, that can check or fix the lines you've edited, or a patch range. There are plugins for various editors and can be integrated into git commit hooks and/or the CI.

Depending on if anybody has any large work outstanding, it might be better to wait to reformat the code base just before or just after a release. I could also work through it a bit more slowly, starting with files that haven't been updated in a while.

https://clang.llvm.org/docs/ClangFormat.html
https://clang.llvm.org/docs/ClangFormatStyleOptions.html

[connortechnology]

In general I'm all for this, however I have held back on even looking into it because there are lots of cases where I think a slightly lax adherence to the rules results in more readable code. Perhaps we can address that through configuration. So let's discuss.

I kinda feel that 160 is too wide. I often feel that 80 is too narrow. Maybe something more like 100 would be better.

We have some cases of things like int x=something, y=somethingelse, z=blah. I never do this myself, but when I see it, I want each of those on a separate line and as we see in this output, clang-format will merge them onto 1 line. So the answer there might be to not use the comma separated definition style.

This also reorders the includes... I was under the impression ours were to go first before the system includes... I'll have to re-read the best practices on that.

I eagerly await other's comments.

[dougnazar]

I am a bit worried it would lead to bikeshedding. I think you can set it when checking to not error for certain formatting issues. Another option is to just run it once to get the code consistent and trust in the reviews to keep it. You can use comments to disable the checker/reformat if needed in a few places but that should probably be kept to a minimum.

I don't see any settings about the variable wrapping. Style guide just says:

It is allowed (if unusual) to declare multiple variables in the same declaration, but it is disallowed if any of those have pointer or reference decorations. Such declarations are easily misread.

For the includes, it is fairly configurable, and is using the Google preferred order:

Include headers in the following order: Related header, C system headers, C++ standard library headers, other libraries' headers, your project's headers.

[Carbenium]

So, here are my two cents:

I'm all in for this. I routinely press the auto-format shortcut in my IDE and just not really spend thoughts on formatting while writing code. IMHO that frees up quite a bit of brain processing power. If we have now the chance to codify and make these rules distributable, that's a great step forward.
Also, in a code review scenario there is no need to spend time on formatting comments, since whatever clang-format says, that's the correct way of doing things. I agree, that there is the possibility of less than optimal formatting in certain cases, but it's at least my gut feeling, that the benefits outweigh this downside (and as @dougnazar mentioned, the formatter can be deactivated in those places).

Some more concrete thoughts on the points mentioned before:

• Line width: no real strong opinions here, other than that it should be more than 80. I normally work on 120
• Include ordering: I/we kinda settled on the opposite ordering than what Google uses: local to global and alphabetical within the category. See Cleanup and reorganize includes  #3127 / 0dbc39e .
• I agree with @connortechnology regarding having variable declaration on separate lines

Just as a sanity check, it might be a good idea to run clang-format on files which have been added or rewritten completely "recently". Those should come out pretty much unchanged, if the clang-format settings match the formatting expectations. The sample size is limited, but I guess better than nothing... So that would be:

• zm_analysis_thread.{h,cpp}
• zm_box.h
• zm_crypt.h
• zm_crypto_generics.h
• zm_crypto_gnutls.h
• zm_crypto_openssl.h
• zm_decoder_thread.{h,cpp}
• zm_define.h
• zm_font.{h,cpp}
• zm_frame.{h,cpp}
• zm_line.h
• zm_poll_thread.{h,cpp}
• zm_poly.{h,cpp}
• zm_time.{h,cpp}
• zm_vector2.h
• zm_zone_stats.h
• and probably pretty much everthing from tests/*

Happy to discuss this further.

[dougnazar]

So here is the above listed files (skipping test). I've adjusted it to 100 columns, and adjusted the include ordering, although I don't think I can group the conditional includes separately the bottom, external library headers (anything with a prefix directory) will be at the bottom.

If these files are closer to the style you want, I can start making adjustments to get the formatting closer.

[connortechnology]

I'd say that in general I like the current settings and resulting formatting.

The only comment I have at the moment is that when we a function with a lot of parameters, instead of two or three long lines, I prefer it to just give each parameter it's own line. Makes adding/subtracting parameters a little easier to read in the diff. I don't know if that is possible, but if it is, then I would do that, and then merge this.

[dougnazar]

I've pushed that change as a diff to make it easier to review.

I'll rebuild the pull request based on master when ready.