Add clang-format configs #323
Conversation
|
@valkey-io/contributors @mattsta |
|
yeah, 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 extra note: may want to double check any recursive auto-formatting does not format the |
|
Can you apply it on a file? That might be easier to check against too for sanity. Maybe something like module.c. |
|
The answer was that single lines attached and multi-lines don't attach. It seems like clang supports it with |
| #define VALKEYMODULE_CTX_TEMP_CLIENT \ | ||
| (1 << 6) /* Return client object to the pool \ | ||
| when the context is destroyed */ |
There was a problem hiding this comment.
This looks very weird, not sure if we need to ignore this or fix it.
There was a problem hiding this comment.
Yeah, that's also a good example of why trailing line comments are bad in practice.
It's much cleaner to do:
/* comment for the next thing */
#define VALKEYMODULE_CTX_TEMP_CLIENT (1 << 6)It would require manually fixing all those broken end-of-line comments though.
There was a problem hiding this comment.
I'm surprised this even compiles. Don't you need to escape the new-line after ...CLIENT ?
There was a problem hiding this comment.
Note: quantified experiments in code-understanding concluded that people understand trailing-line comments more easily. Sorry, I learned that so long ago that I don't have a reference right now.
There was a problem hiding this comment.
Don't you need to escape the new-line after ...CLIENT ?
if you scroll far to the right, the \ is right-aligned at full line spec width (the github preview just has it out of scope with no wide scroll indicator)
experiments in code-understanding concluded that people understand trailing-line comments more easily.
seems illogical? vertical parsing is clearly superior.
also this example is weird because it's a macro. including a comment in-line with a macro means the comment is also being expanded into the macro replacement locations (which works, but is also unnecessary).
There was a problem hiding this comment.
Extremely well thought through 👍
Yeah, your 5 vs 6 example is good because it depends on the use case. It's nice seeing the vertical alignment to compare values, but the per-line comments overflow the viewport; while the 6 is easier to read the comments but takes an extra second to very 5->6->7 are in order.
Overall, going for nice machine style enforcing clean maintainable readability is always a win over "big programmer brain decide style decision per-line" (at least when working in groups and on projects that can live for decades, for single projects just do whatever, which is why it's always nice to have single projects too!).
overall these are just feedback ideas for whoever makes actual decisions around here 🦅
values!
This is also a bit of a weird/contrived example because technically these things should be enums and not defines anyway 🙃
oh glob i just looked and the enums in the code are all weird too. They are almost all anonymous typedef enum { a, b, c } name; so they won't show their fully type details in debuggers versus proper usage of typedef enum name { a, b, c } name; wheeeee:
src/$ rg "typedef enum" --stats
37 matches
37 matched lines
18 files contained matchesalignment
as for right-aligned continues, here's some live code found in the wild:
#define INSERT_SEP \
if (g->state[g->depth] == yajl_gen_map_key || \
g->state[g->depth] == yajl_gen_in_array) { \
g->print(g->ctx, ",", 1); \
if ((g->flags & yajl_gen_beautify)) g->print(g->ctx, "\n", 1); \
} else if (g->state[g->depth] == yajl_gen_map_val) { \
g->print(g->ctx, ":", 1); \
if ((g->flags & yajl_gen_beautify)) g->print(g->ctx, " ", 1); \
}
#define INSERT_WHITESPACE \
if ((g->flags & yajl_gen_beautify)) { \
if (g->state[g->depth] != yajl_gen_map_val) { \
unsigned int _i; \
for (_i=0;_i<g->depth;_i++) \
g->print(g->ctx, \
g->indentString, \
(unsigned int)strlen(g->indentString)); \
} \
}
#define ENSURE_NOT_KEY \
if (g->state[g->depth] == yajl_gen_map_key || \
g->state[g->depth] == yajl_gen_map_start) { \
return yajl_gen_keys_must_be_strings; \
} \
/* initialize a bytestack */
#define yajl_bs_init(obs, _yaf) { \
(obs).stack = NULL; \
(obs).size = 0; \
(obs).used = 0; \
(obs).yaf = (_yaf); \
} \
/* initialize a bytestack */
#define yajl_bs_free(obs) \
if ((obs).stack) (obs).yaf->free((obs).yaf->ctx, (obs).stack);
#define yajl_bs_current(obs) \
(assert((obs).used > 0), (obs).stack[(obs).used - 1])
#define yajl_bs_push(obs, byte) { \
if (((obs).size - (obs).used) == 0) { \
(obs).size += YAJL_BS_INC; \
(obs).stack = (obs).yaf->realloc((obs).yaf->ctx,\
(void *) (obs).stack, (obs).size);\
} \
(obs).stack[((obs).used)++] = (byte); \
}and here's a better auto-format cleaned-up version (still not perfect; executable code statements in defines should be in a do { } while (0); loop, etc, but that's behavior/correctness issues instead of formatting):
#define INSERT_SEP \
if (g->state[g->depth] == yajl_gen_map_key || \
g->state[g->depth] == yajl_gen_in_array) { \
g->print(g->ctx, ",", 1); \
if ((g->flags & yajl_gen_beautify)) \
g->print(g->ctx, "\n", 1); \
} else if (g->state[g->depth] == yajl_gen_map_val) { \
g->print(g->ctx, ":", 1); \
if ((g->flags & yajl_gen_beautify)) \
g->print(g->ctx, " ", 1); \
}
#define INSERT_WHITESPACE \
if ((g->flags & yajl_gen_beautify)) { \
if (g->state[g->depth] != yajl_gen_map_val) { \
unsigned int _i; \
for (_i = 0; _i < g->depth; _i++) \
g->print(g->ctx, g->indentString, \
(unsigned int)strlen(g->indentString)); \
} \
}
#define ENSURE_NOT_KEY \
if (g->state[g->depth] == yajl_gen_map_key || \
g->state[g->depth] == yajl_gen_map_start) { \
return yajl_gen_keys_must_be_strings; \
}
/* initialize a bytestack */
#define yajl_bs_init(obs, _yaf) \
{ \
(obs).stack = NULL; \
(obs).size = 0; \
(obs).used = 0; \
(obs).yaf = (_yaf); \
}
/* initialize a bytestack */
#define yajl_bs_free(obs) \
if ((obs).stack) \
(obs).yaf->free((obs).yaf->ctx, (obs).stack);
#define yajl_bs_current(obs) \
(assert((obs).used > 0), (obs).stack[(obs).used - 1])
#define yajl_bs_push(obs, byte) \
{ \
if (((obs).size - (obs).used) == 0) { \
(obs).size += YAJL_BS_INC; \
(obs).stack = (obs).yaf->realloc((obs).yaf->ctx, \
(void *)(obs).stack, (obs).size); \
} \
(obs).stack[((obs).used)++] = (byte); \
}There was a problem hiding this comment.
Speaking of
big programmer brain decide style decision per-line
see https://en.wikipedia.org/wiki/Donald_Knuth and https://en.wikipedia.org/wiki/Literate_programming
There was a problem hiding this comment.
This is just an automation tool doing its magic.
I would prefer not having trailing comments but I don't know if clang-format can handle that.
clang-format is the best free tool that I know of so I am willing to work with its limitations in exchange of a consistent style (even if it is not my favorite). In other words, you can say my bar is low, just "consistency" :-)
There was a problem hiding this comment.
But we can decide the our line limit is longer than 80, which is a very old limitation dating back to CRT monitors and VI vs Emacs wars.
I can easily live with 120 or even 160 line limit.
To make a point my current screen is so wide (and curved) that most of this page is white space to either side and a narrow band of thread in the middle.
There was a problem hiding this comment.
I actually like 80, it imposes a certain discipline. I'm OK with 120, but not 160. At 160 and at the font-size that I need, there isn't room for much else on my screen, and I like to look at references and stuff without hiding the actual code.
| * ValkeyModule_ChannelAtPosWithFlags(ctx, 1, VALKEYMODULE_CMD_CHANNEL_SUBSCRIBE | VALKEYMODULE_CMD_CHANNEL_PATTERN); | ||
| * ValkeyModule_ChannelAtPosWithFlags(ctx, 1, VALKEYMODULE_CMD_CHANNEL_PUBLISH); | ||
| * ValkeyModule_ChannelAtPosWithFlags(ctx, 1, VALKEYMODULE_CMD_CHANNEL_SUBSCRIBE | | ||
| * VALKEYMODULE_CMD_CHANNEL_PATTERN); ValkeyModule_ChannelAtPosWithFlags(ctx, 1, VALKEYMODULE_CMD_CHANNEL_PUBLISH); |
There was a problem hiding this comment.
This is another weird side effect, it's trying to make it like a real sentence, when we just want it to look like commented code. We'll probably want to wrap all of the module docs with this. We probably want to take extra care here.
There was a problem hiding this comment.
yeah, fix is either allow the long lines to linger (but they were probably written too long in the first place) or run around and fix them all into the project style width requirements 🤷
or go for extreme reduction and rename ValkeyModule -> VKM everywhere to help with shorter lines 🙈
There was a problem hiding this comment.
clang-format doesn't seem to know how to format code embedded in the comments so yeah we will have to manually format it for once. But there shouldn't be more work after that.
src/.clang-format
Outdated
| SpacesInSquareBrackets: false | ||
| ReflowComments: true | ||
| CommentPragmas: '^\\s*\\*' | ||
| InsertBraces: true |
There was a problem hiding this comment.
| InsertBraces: true | |
| InsertBraces: false |
I don't feel like we should force this, I see it is causing a lot of changes.
There was a problem hiding this comment.
"optional syntax" in different places like un-braced if/while/do/for statements are an easy way to have bugs slip in by accident so it probably shouldn't be allowed. Much better for standard reliability development practices to enforce braces everywhere.
Typical arguments against it are:
- but we enforce it with formatting (weak argument in a non-space-sensitive language)
- but i don't want to (weak argument)
- but i want statements only on one line (weak argument, makes updating/refactoring difficult)
everybody, at some point, has done a change like this when in a hurry and broken something. The easy fix is to just not allow it and enforce it with coding styles:
if (thing)
a += 1;
if (thing)
printf("CHECK: %d\n", a);
a += 1;Or, you have a one line if statement and you want to paste something else in there for structure, so you have to re-add braces when it would cost nothing to just use them in the first place?
Also without braces, it makes quick "capture braces for cut/paste" moving/refactoring more difficult too.
There was a problem hiding this comment.
On the good side, the existing code is so totally random with its braces that the programmer is probably ready for this sh*t.
There was a problem hiding this comment.
if (thing)
a += 1;
vs
if (thing) a+= 1;
Is actually the format I use fairly frequently, since I think when used appropriately it makes code a lot more readable. A case in module.c was:
if (clusterNodeIsMyself(node)) *flags |= VALKEYMODULE_NODE_MYSELF;
if (clusterNodeIsMaster(node)) *flags |= VALKEYMODULE_NODE_PRIMARY;
if (clusterNodeIsSlave(node)) *flags |= VALKEYMODULE_NODE_REPLICA;
if (clusterNodeTimedOut(node)) *flags |= VALKEYMODULE_NODE_PFAIL;
if (clusterNodeIsFailing(node)) *flags |= VALKEYMODULE_NODE_FAIL;
if (clusterNodeIsNoFailover(node)) *flags |= VALKEYMODULE_NODE_NOFAILOVER;
I could give it up though.
There was a problem hiding this comment.
I would never complain about the last example in a code review.
If you asked me, I would vote against enforcing braces (or the equivalent for each language), but I know that nearly every coding standard in the world does insist on them.
If you are being forced to use braces, you can always write these examples as
if (x) { *flags |= y; }
Sadly, that too is disallowed by every set of coding standards I have ever seen.
The best advice I have ever seen came from a paper I read in the 90's. It recognized that different people respond in different ways to the same thing, so we should store all programs in a canonical format, but display them and edit them in whatever format the user likes. Sadly, even today our formatters are not good enough to support this.
There was a problem hiding this comment.
Warnings like that scare me.
If for some reason we choose to enable that option, then we really should take the recommended extra care. But that would require that we get some kind of alert when it happens. Does such an alert get generated by the formatter?
There was a problem hiding this comment.
@madolson "I don't feel like we should force this, I see it is causing a lot of changes." lets rip the band-aid once...
besides, git now days knows to look behind marked commits and blame the correct person around the re-formatted code
So lets decide on what we like and less on what we have
There was a problem hiding this comment.
My point is more that a lot of people prefer this. I don't like forcing style that is harder to read. I don't like forcing the braces.
There was a problem hiding this comment.
If we want to introduce this incrementally, we should start with more liberal rules. Then we can make them more strict later on.
There was a problem hiding this comment.
If we want to introduce this incrementally, we should start with more liberal rules. Then we can make them more strict later on.
I like this.
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 |
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 |
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.
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?
I am willing to compromise on 2 to get 1. @valkey-io/core-team |
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
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 |
|
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 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. |
|
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. |
As you wish :-) |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -148,7 +147,7 @@ | |||
| dictEntry *samples[server.maxmemory_samples]; | |||
|
|
|||
| int slot = kvstoreGetFairRandomDictIndex(samplekvs); | |||
| count = kvstoreDictGetSomeKeys(samplekvs,slot,samples,server.maxmemory_samples); | |||
| count = kvstoreDictGetSomeKeys(samplekvs, slot, samples, server.maxmemory_samples); | |||
Check failure
Code scanning / CodeQL
Use of a broken or risky cryptographic algorithm
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #323 +/- ##
============================================
- Coverage 69.81% 68.05% -1.77%
============================================
Files 109 109
Lines 61801 63657 +1856
============================================
+ Hits 43149 43321 +172
- Misses 18652 20336 +1684
|
src/networking.c
Outdated
| if (listLength(src->reply)) { | ||
| listJoin(dst->reply, src->reply); | ||
| } |
There was a problem hiding this comment.
It feels like every other change is this. I feel strongly now that we shouldn't enforce that everything is wrapped, because we can skip it right?
|
Reposting an earlier comment:
That means: and |
|
I went through the whole diff and identified where I think we need to disable the automatic format. I added With these exceptions, I'm fine with running clang-format on the rest. 👍 |
|
I think we need to clarify under which condition or provide a reference link , developer should use /* clang-format off */ flag? |
This is a preparation for adding clang-format. These comments prevent automatic formatting in some places. With these exceptions, we will be able to run clang-format on the rest of the code. This is a preparation for #323. --------- Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
src/.clang-format
Outdated
| PenaltyExcessCharacter: 100 | ||
| MaxEmptyLinesToKeep: 2 | ||
| BreakBeforeBraces: Attach | ||
| AllowShortIfStatementsOnASingleLine: true |
There was a problem hiding this comment.
It doesn't seem like true is one of the possible values of this config. https://clang.llvm.org/docs/ClangFormatStyleOptions.html#allowshortifstatementsonasingleline
If we want to allow it for if without else, we could use
| AllowShortIfStatementsOnASingleLine: true | |
| AllowShortIfStatementsOnASingleLine: WithoutElse |
There was a problem hiding this comment.
I think we should also enable
AllowShortCaseLabelsOnASingleLine: true
|
sorry - human error. will re-open |
Signed-off-by: Ping Xie <pingxie@google.com>
|
single lines preserved. changes are mostly around spaces (missing spaces, extra spaces, trailing spaces, etc). |
src/server.c
Outdated
| if (server.current_client && | ||
| server.current_client->cmd && | ||
| server.current_client->cmd->flags & CMD_TOUCHES_ARBITRARY_KEYS) | ||
| { | ||
| if (server.current_client && server.current_client->cmd && | ||
| server.current_client->cmd->flags & CMD_TOUCHES_ARBITRARY_KEYS) { | ||
| transaction = 0; | ||
| } |
There was a problem hiding this comment.
What happened to the attach multiline rule?
In general, I think it's much better now and the total changed lines are much less: 30k instead of 40k.
Btw, you didn't commit the clang format config file?
There was a problem hiding this comment.
added .clang-format.
What happened to the attach multiline rule?
it is attached, right?
There was a problem hiding this comment.
Yes, but I thought we wanted to attach single line but detach when multiline, BraceWrappingAfterControlStatementStyle MultiLine. That was the old convention anyway and I think most of the code followed this style, but I may have missed some discussion here and it's no big deal for me. I just noticed the change.
There was a problem hiding this comment.
Got it. We will always attach.
There was a problem hiding this comment.
I was noticing the number of multi-line control statements has gone down significantly, so attaching the braces probably is just a lot less important.
There was a problem hiding this comment.
Yes, anyway, my concern was always that there's no indentation difference between secondlongline and thirdlongline in the example below, but now I've come to the conclusion that this is never really a problem. The important thing is that it's easy to see where the whole if starts and ends (}).
if (alonglongline ||
secondlongline) { // <--
thirdlongline; // <-- same indentation
morelines;
}There was a problem hiding this comment.
Oh, that's strange. Most formatters that I have seen indent secondlongline one more time.
| size_t | ||
| lzf_compress (const void *const in_data, size_t in_len, | ||
| void *out_data, size_t out_len); | ||
| size_t lzf_compress(const void *const in_data, size_t in_len, void *out_data, size_t out_len); |
There was a problem hiding this comment.
The .clang-format-ignore file I added in the other PR didn't work?
There was a problem hiding this comment.
hmm - don't know why. let me revert all changes and reapply the formatting.
There was a problem hiding this comment.
I think my clang-format is too old. It is 14.0. I have been getting all kinds of errors when trying to update it on my machines. Will need to find a different machine and try again.
There was a problem hiding this comment.
This can be a problem for users I suppose. An old version reformats lots of files and it gets committed. We could add the comment /* clang-formt off */ in all these files instead if it's safer.
This is a preparation for adding clang-format. These comments prevent automatic formatting in some places. With these exceptions, we will be able to run clang-format on the rest of the code. This is a preparation for valkey-io#323. --------- Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: Karthick Ariyaratnam <karthyuom@gmail.com>
Signed-off-by: Ping Xie <pingxie@google.com>
So much for consistency. |
| /* Everything below this line is automatically generated by | ||
| * generate-fmtargs.py. Do not manually edit. */ |
There was a problem hiding this comment.
This code is automatically generated, we need to fix the generator too.
There was a problem hiding this comment.
Hehe, we can make the generator emit /* clang-format off */.
There was a problem hiding this comment.
Yeah, I think that makes more sense than generating compliant code.
src/functions.c
Outdated
| " * ASYNC: Asynchronously flush the libraries.", | ||
| " * SYNC: Synchronously flush the libraries.", | ||
| "DUMP", | ||
| " Return a serialized payload representing the current libraries, can be restored using FUNCTION RESTORE command", |
There was a problem hiding this comment.
I found it difficult to see what the problem was, so I will add some details. The original looks like this:
" Return a serialized payload representing the current libraries, can be restored using FUNCTION RESTORE command",
After autoformatting it looks like this:
" Return a serialized payload representing the current libraries, can be restored using FUNCTION RESTORE "
"command",
I agree that this is a step backward. Another fine example of why I would prefer that the formatting be done by the developer prior to commit, and any time the formatter actually changes something it fails that stage of the pipeline.
There was a problem hiding this comment.
I would prefer that the formatting be done by the developer prior to commit, and any time the formatter actually changes something it fails that stage of the pipeline.
Yeah that's how it will work once we've formatted all the old code. IIUC.
We should fix long lines like that manually, preferably in separate PRs before we autoformat everything.
src/.clang-format
Outdated
| SpacesInSquareBrackets: false | ||
| ReflowComments: true | ||
| CommentPragmas: '^\\s*\\*' | ||
| InsertBraces: true |
There was a problem hiding this comment.
Warnings like that scare me.
If for some reason we choose to enable that option, then we really should take the recommended extra care. But that would require that we get some kind of alert when it happens. Does such an alert get generated by the formatter?
| #define VALKEYMODULE_CTX_TEMP_CLIENT \ | ||
| (1 << 6) /* Return client object to the pool \ | ||
| when the context is destroyed */ |
There was a problem hiding this comment.
I actually like 80, it imposes a certain discipline. I'm OK with 120, but not 160. At 160 and at the font-size that I need, there isn't room for much else on my screen, and I like to look at references and stuff without hiding the actual code.
I have validated that these settings closely match the existing coding style with one major exception on
BreakBeforeBraces, which will beAttachgoing forward. The mixedBreakBeforeBracesstyles in the current codebase are hard to imitate and also very odd IMHO - see belowPlease do NOT merge just yet. Will add the github action next once the style is reviewed/approved.