Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clang-format: set ColumnLimit to 0 and reformat #1045

Merged
merged 2 commits into from
Sep 24, 2024

Conversation

mkmkme
Copy link
Contributor

@mkmkme mkmkme commented Sep 18, 2024

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.

Copy link

codecov bot commented Sep 18, 2024

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 ⚠️
src/sentinel.c 0.00% 1 Missing ⚠️
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%) ⬇️
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

@bjosv
Copy link
Contributor

bjosv commented Sep 18, 2024

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.

@hpatro hpatro requested a review from PingXie September 18, 2024 18:59
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this makes clang-format much less annoying. It lets us remove the /* clang-format off */ which is a good indication that it respects our style better.

ColumnLimit: 0 lets the programmer decide where to make line breaks, right? This seems more practical. Clang-format can help with other things.

@PingXie @madolson WDYT?

src/ae.h Outdated Show resolved Hide resolved
src/atomicvar.h Outdated Show resolved Hide resolved
@mkmkme
Copy link
Contributor Author

mkmkme commented Sep 19, 2024

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
Copy link
Contributor Author

mkmkme commented Sep 20, 2024

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.

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:

```c
\#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.

Signed-off-by: Mikhail Koviazin <[email protected]>
Signed-off-by: Mikhail Koviazin <[email protected]>
@PingXie
Copy link
Member

PingXie commented Sep 20, 2024

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.

@mkmkme
Copy link
Contributor Author

mkmkme commented Sep 20, 2024

@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
Copy link
Contributor Author

mkmkme commented Sep 20, 2024

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

Copy link
Member

@PingXie PingXie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @mkmkme!

src/t_string.c Show resolved Hide resolved
Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me!

@zuiderkwast zuiderkwast merged commit af81174 into valkey-io:unstable Sep 24, 2024
47 checks passed
@mkmkme mkmkme deleted the mkmkme/column-limit-0 branch September 25, 2024 07:12
naglera pushed a commit to naglera/placeholderkv that referenced this pull request Oct 10, 2024
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:

```c
#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.

---------

Signed-off-by: Mikhail Koviazin <[email protected]>
Signed-off-by: naglera <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants