Add clang-format configs

[PingXie]

I have validated that these settings closely match the existing coding style with one major exception on BreakBeforeBraces, which will be Attach going forward. The mixed BreakBeforeBraces styles in the current codebase are hard to imitate and also very odd IMHO - see below

if (a == 1) { /*Attach */
}

if (a == 1 ||
    b == 2)
{ /* Why? */
}

Please do NOT merge just yet. Will add the github action next once the style is reviewed/approved.

[PingXie]

@valkey-io/contributors @mattsta

[mattsta]

yeah, BreakBeforeBraces: Attach looks correct. Removing any custom indent matching or function art is cleaner. Cleaning up inconsistencies looks good even if it changes some of the current style decisions (which is the point of more stable auto-formatted styles!).

and if there is a need for specific visible alignment somewhere, just wrapping in ignore/resume blocks works:

/* clang-format off */
// my weird code i don't want the formatter to touch
/* clang-format on */

Sometimes wrapping byte tables or other bulk data in ignore blocks helps if the formatter tries to turn an 80 column array into 500 lines only having one element each or something else weird.

Most of the settings are already provided by the LLVM style preference from BasedOnStyle: LLVM (also see: clang-format -style=llvm -dump-config), but being explicit with extra options is always fine too.

extra note: may want to double check any recursive auto-formatting does not format the deps directory because that would probably be a big mess unnecessarily.

[madolson]

Can you apply it on a file? That might be easier to check against too for sanity. Maybe something like module.c.

[madolson]

The answer was that single lines attached and multi-lines don't attach. It seems like clang supports it with BraceWrappingAfterControlStatementStyle MultiLine. I came to like it more than attaching or detaching in all cases. I think this more closely matches the current style if we want to keep it.

[daniel-house]

Can you apply it on a file? That might be easier to check against too for sanity. Maybe something like module.c.

Can you apply it on a paragraph from inside your favorite editor?

I currently do all my code-formatting inside Eclipse using a minor tweak of the default K&R formatting rules. I can highlight a few lines, then hit ctrl-shift-f, and see if I like the result. In rare cases the result is unacceptable and I edit the code a little bit more by hand. This means I can get automated formatting without affecting any of the code I didn't change, so the code review doesn't report a lot of lines changed when I never touched them.

Eclipse: https://marketplace.eclipse.org/free-tagging/clang-format

Vim: see the Vim section of https://clang.llvm.org/docs/ClangFormat.html

[mattsta]

Can you apply it on a paragraph from inside your favorite editor?

That's another approach reasonable too: only require formatting on updated things.

the formatter can be made diff/patch-aware so it only checks changed code: https://clang.llvm.org/docs/ClangFormat.html#script-for-patch-reformatting

[PingXie]

Can you apply it on a paragraph from inside your favorite editor?

Not sure if you are looking for an example or it is more about a feature but yes many editors allow to you change selected text only.

only require formatting on updated things.

I think we have already gone past that point. The whole rename exercise pretty much killed half of the code history (and more is coming). I would say we push forward now and reformat the whole codebase to whatever we like. I am worried that the window for reformatting is closing with us merging more PRs.

Can we take a vote on the two decisions below?

1. clang-format the entire codebase and create an automation via github actions for every merge (Valkey code only)
2. the exact style to use

I am willing to compromise on 2 to get 1.

@valkey-io/core-team

[zuiderkwast]

clang-format the entire codebase

I would like to se a diff of the whole change to decide if I like it or not.

If it's a lot of reformatting, then I'd prefer only doing it changed lines.

The rebranding didn't touch many lines. For example, in src/server.c, we only changed 4.5% of the lines since forking. The blame info is still usable.

the exact style to use

I think we should use style rules that cause the least changes to the code base. Why? We already have conventions, even though they're not written down and are not enforced by tools.

For example, this style is fairly common in the code base so I think we should keep it, without braces and without extra newlines:

    if (x) flags |= FLAG_X;
    if (y) flags |= FLAG_Y;
    ...

I'd be fine with enclosing blocks of such code with /* clang-format off */ and /* clang-format on */ comments though. The same probably applies to many long printf-like lines (INFO command), especially where we use the FMTARGS macro.

[daniel-house]

I would feel much better if, instead of auto-formatting every commit, we added a new format-test to the CI pipeline. This test would fail if applying the auto-format changed anything.

My thinking is, first and foremost, we are smarter than the auto-formatter. It will all be code reviewed by humans any way, and if the humans approve it then it shouldn't be changed by the formatter. If we review it after the auto-formatter hits it, and we don't like the way it looks, it may be difficult to recognize that the ugliness was introduced by the auto-formatter. On the other hand, if the author knowingly submitted code that the auto-formatter would change, the author would be required to flag it with the dont-format-this annotation, which would in turn alert reviewers that the author intentionally did something unusual. Ideally, we should almost always accept the results of the auto-formatter, but still have room to do those rare things that we do best.

This approach also ensures that the author of a change looks at the after-formatted code and has a chance to improve it before wasting a reviewer's time.

[daniel-house]

Please make sure the auto-format rules are idempotent. I have seen some formatters that don't like their own output. If we are going to auto-format the entire code base, we could run a simple test to ensure that at least the current result is a fixed-point of the auto-format function.

[PingXie]

I would like to se a diff of the whole change to decide if I like it or not.

As you wish :-)

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 81.98640% with 477 lines in your changes are missing coverage. Please review.

Project coverage is 70.15%. Comparing base (c478206) to head (a3e80fc).

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #323      +/-   ##
============================================
+ Coverage     69.71%   70.15%   +0.43%     
============================================
  Files           109      109              
  Lines         61864    59904    -1960     
============================================
- Hits          43131    42028    -1103     
+ Misses        18733    17876     -857     

Files	Coverage Δ
src/acl.c	88.89% <ø> (-0.09%)	:arrow_down:
src/blocked.c	91.87% <100.00%> (+0.06%)	:arrow_up:
src/childinfo.c	95.89% <100.00%> (+1.29%)	:arrow_up:
src/cluster_legacy.c	86.30% <ø> (+0.15%)	:arrow_up:
src/config.c	78.31% <ø> (+0.43%)	:arrow_up:
src/connection.h	93.50% <100.00%> (-0.09%)	:arrow_down:
src/connhelpers.h	100.00% <ø> (ø)
src/debug.c	53.94% <ø> (+0.47%)	:arrow_up:
src/function_lua.c	99.17% <100.00%> (ø)
src/functions.c	95.58% <100.00%> (-0.04%)	:arrow_down:
... and 80 more

... and 1 file with indirect coverage changes

[madolson]

Reposting an earlier comment:

The answer was that single lines attached and multi-lines don't attach. It seems like clang supports it with BraceWrappingAfterControlStatementStyle MultiLine. I came to like it more than attaching or detaching in all cases. I think this more closely matches the current style if we want to keep it.

That means:

if (foo) {

and

if (foo && whateverthisthingiswaytoolongomgwhatareyoudoingcomeupwithabettername ||
    is notanybetter)
{

[zuiderkwast]

I went through the whole diff and identified where I think we need to disable the automatic format. I added /* clang-format off */ annotations in #468. We can merge it as a preparation, before the run clang-format. Feel free to add more exceptions.

With these exceptions, I'm fine with running clang-format on the rest. :+1:

[hwware]

I think we need to clarify under which condition or provide a reference link , developer should use /* clang-format off */ flag? I think we should describe this in some places so that all people can follow this rule

[PingXie]

sorry - human error. will re-open

[PingXie]

single lines preserved. changes are mostly around spaces (missing spaces, extra spaces, trailing spaces, etc).

[daniel-house]

Reposting an earlier comment:

The answer was that single lines attached and multi-lines don't attach. It seems like clang supports it with BraceWrappingAfterControlStatementStyle MultiLine. I came to like it more than attaching or detaching in all cases. I think this more closely matches the current style if we want to keep it.

That means:

if (foo) {

and

if (foo && whateverthisthingiswaytoolongomgwhatareyoudoingcomeupwithabettername ||
    is notanybetter)
{

So much for consistency.

[PingXie]

Also sharing core-team discussion: once this PR is merged, we will create a new github action to block future PRs if the new clang-format action ends up modifying the PRs.

[PingXie]

@valkey-io/core-team can you please approve this PR?

[PingXie]

We need to add the CI job ASAP after merging this.

This is my first github action. Let me know if I missed anything #538