valkey icon indicating copy to clipboard operation
valkey copied to clipboard

clang-format: set ColumnLimit to 0 and reformat

Open mkmkme opened this issue 1 year ago • 7 comments

This commit hopefully improves the formatting of the codebase by setting ColumnLimit to 0 and hence stopping clang-format from trying to put as much stuff in one line as possible.

This change enabled us to remove most of clang-format off directives and fixed a bunch of lines that looked like this:

#define KEY \
    VALUE /* comment */

Additionally, one pair of clang-format off / clang-format on had clang-format off as the second comment and hence didn't enable the formatting for the rest of the file. This commit addresses this issue as well.

Please tell me if anything in the changes seem off. If everything is fine, I will add this commit to .git-blame-ignore-revs later.

mkmkme avatar Sep 18 '24 07:09 mkmkme

Codecov Report

Attention: Patch coverage is 98.28571% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.58%. Comparing base (ba71c7e) to head (b8e39e5). Report is 11 commits behind head on unstable.

Files with missing lines Patch % Lines
src/t_string.c 98.60% 2 Missing :warning:
src/sentinel.c 0.00% 1 Missing :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #1045      +/-   ##
============================================
- Coverage     70.62%   70.58%   -0.04%     
============================================
  Files           114      114              
  Lines         61672    61684      +12     
============================================
- Hits          43555    43541      -14     
- Misses        18117    18143      +26     
Files with missing lines Coverage Δ
src/acl.c 88.89% <ø> (ø)
src/ae.c 74.90% <ø> (ø)
src/asciilogo.h 100.00% <ø> (ø)
src/cluster_legacy.c 85.85% <ø> (-0.48%) :arrow_down:
src/cluster_slot_stats.c 94.04% <ø> (ø)
src/config.c 78.69% <ø> (ø)
src/debug.c 53.67% <ø> (ø)
src/dict.c 97.26% <ø> (ø)
src/eval.c 56.54% <ø> (ø)
src/expire.c 96.42% <ø> (ø)
... and 30 more

... and 6 files with indirect coverage changes

codecov[bot] avatar Sep 18 '24 07:09 codecov[bot]

Nice. Its a bit annoying that clang-format had some formatting changes somewhere between 18.1.3 and 18.1.6. Maybe that the reason for the failing CI check? The check seem to use clang-format 18.1.8.

bjosv avatar Sep 18 '24 10:09 bjosv

Nice. Its a bit annoying that clang-format had some formatting changes somewhere between 18.1.3 and 18.1.6. Maybe that the reason for the failing CI check? The check seem to use clang-format 18.1.8.

Yeah, good point. I forgot to pin the clang-format version when making this change.

Basically I would suggest pinning the clang-format version both in CI and in the documentation (and for developers) because it changes within the patch versions (there will be a difference between 18.1.3 and there is a difference between 18.1.6 and 18.1.8)

mkmkme avatar Sep 19 '24 08:09 mkmkme

Actually now I realized that the large diff in the CI was caused not by the version mismatch, but by the typo in my formatting script that caused only .h files to be reformatted automatically.

My proposal regarding the clang-format version is following: keep the things in CI the way they are right now (so, clang-format will be installed from apt.llvm.org) and observe if there are any issues about the formatting in the future.

If there are more backwards-incompatible changes with patch version of clang-format (like it was between 18.1.3 and 18.1.6), I will create a new pull request with pinning clang-format version.

mkmkme avatar Sep 20 '24 06:09 mkmkme

Yeah I like this change too. Now thinking back the column limit is actually net negative as it forced us to spread clang-format off all over the codebase.

re - version compat. I did get into a lot of issues when switching between version 18 and 19 but I would be surprised if a patch version upgrade breaks things. that said, I see no harm pinning to a specific patch version.

Thanks for making this change, @mkmkme! Please let me know when this PR is ready for the final review.

PingXie avatar Sep 20 '24 06:09 PingXie

@PingXie

but I would be surprised if a patch version upgrade breaks things

It did. I had incompatibility issues between 18.1.3 and 18.1.6 in libvalkey PR

mkmkme avatar Sep 20 '24 06:09 mkmkme

Also I can see that "Clang Format Check" got its green mark. So @PingXie @zuiderkwast @bjosv this is now ready for a proper review :)

mkmkme avatar Sep 20 '24 06:09 mkmkme