-
Notifications
You must be signed in to change notification settings - Fork 645
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
Conversation
fb494f4
to
a996239
Compare
Codecov ReportAttention: Patch coverage is
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
|
Nice. |
There was a problem hiding this 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.
Yeah, good point. I forgot to pin the Basically I would suggest pinning the |
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 My proposal regarding the If there are more backwards-incompatible changes with patch version of |
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]>
c0afd62
to
b8e39e5
Compare
Yeah I like this change too. Now thinking back the column limit is actually net negative as it forced us to spread 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. |
It did. I had incompatibility issues between 18.1.3 and 18.1.6 in libvalkey PR |
Also I can see that "Clang Format Check" got its green mark. So @PingXie @zuiderkwast @bjosv this is now ready for a proper review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @mkmkme!
There was a problem hiding this 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!
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]>
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:Additionally, one pair of
clang-format off
/clang-format on
hadclang-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.