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.
If you have to scroll all the way to the maximum line width in order to find something
It's kinda an artifact of somebody setting the line width unnaturally long too. Normally you just set 80 chars and everything looks fine (and normally we don't develop in github quoted preview blocks etc).
As for end of line comments, here we go using standard tools:
There was a problem hiding this comment.
Sorry, I don't believe anyone would write end-of-line comments like that. That looks like a straw-man argument to me.
Suppose the original code looks like this:
#define VALKEYMODULE_CTX_TEMP_CLIENT (1 << 6) /* Return client object to the pool when the context is destroyed */
-
This is already terrible code!
-
Yes, this would look MUCH better:
/* Return client object to the pool when the context is destroyed */
#define VALKEYMODULE_CTX_TEMP_CLIENT (1 << 6)
- If it is going to be auto-formatted, then this would be horrible but tolerable for me:
#define VALKEYMODULE_CTX_TEMP_CLIENT \
(1 << 6) /* Return client object to the pool \
when the context is destroyed */
But I would much prefer this horrible result:
#define VALKEYMODULE_CTX_TEMP_CLIENT \
(1 << 6) /* Return client object to the pool \
when the context is destroyed */
- This still feels like a straw-man to me. Even a modestly smarter formatter should give
12345678901234567890123456789012345678901234567890123456789012345678901234567890
#define VALKEYMODULE_CTX_TEMP_CLIENT (1 << 6) /* Return client object to the \
pool when the context is \
destroyed */
Note: the first line is a ruler. It is exactly 80 characters long.
- Is (4) more readable than (2)? That probably depends on both the context and the reader. Consider this context:
12345678901234567890123456789012345678901234567890123456789012345678901234567890
#define VALKEYMODULE_CTX_JUNK (1 << 5) /* Do something with junk objects. */
#define VALKEYMODULE_CTX_TEMP_CLIENT (1 << 6) /* Return client object to the pool when the context is destroyed */
#define VALKEYMODULE_CTX_TRASH (1 << 7) /* Don't forget to take out the trash every Thursday night. */
It violates the 80 character rule. In this case, I would say violating the rule is justified.
- Would you prefer this?
/* Do something with junk objects. */
#define VALKEYMODULE_CTX_JUNK (1 << 5)
/* Return client object to the pool when the context is destroyed */
#define VALKEYMODULE_CTX_TEMP_CLIENT (1 << 6)
/* Don't forget to take out the trash every Thursday night. */
#define VALKEYMODULE_CTX_TRASH (1 << 7)
- In summary,
- Communication is HARD.
- Guidelines are good, absolute rules are bad.
- Context is critical.
- Different people respond in different ways to the same thing.
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" :-)
| * 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.
| 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.
I always use {} for statements that could be written on a single line - because if we ended up adding a new line there the change would be smaller and clearer. that said, I am totally fine with trading it in for consistency-via-clang-format.
There was a problem hiding this comment.
that said, I am totally fine with trading it in for consistency-via-clang-format.
I'm really not. I don't think this is worth enforcing. It's a huge chunk of the lines getting changed.
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. |
Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@google.com>
Signed-off-by: Ping Xie <pingxie@google.com>
This reverts commit ffab67b.
Signed-off-by: Ping Xie <pingxie@google.com>
As you wish :-) |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -1239,12 +1340,14 @@ | |||
| #define GETFAIR_NUM_ENTRIES 15 | |||
| dictEntry *dictGetFairRandomKey(dict *d) { | |||
| dictEntry *entries[GETFAIR_NUM_ENTRIES]; | |||
| unsigned int count = dictGetSomeKeys(d,entries,GETFAIR_NUM_ENTRIES); | |||
| unsigned int count = dictGetSomeKeys(d, entries, GETFAIR_NUM_ENTRIES); | |||
Check failure
Code scanning / CodeQL
Use of a broken or risky cryptographic algorithm High
There was a problem hiding this comment.
This is a perfect example of why I don't want to allow a computer to have full control over ... my car ... my meals ... my code's format ... or much of anything. Yes, if it were being used for cryptography it would be bad, but that's not what it is being used for.
| @@ -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 High
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #323 +/- ##
============================================
- Coverage 68.95% 68.05% -0.91%
============================================
Files 109 109
Lines 61793 63657 +1864
============================================
+ Hits 42611 43321 +710
- Misses 19182 20336 +1154
|
| @@ -61,73 +61,79 @@ | |||
| #define MM 156 | |||
| #define MATRIX_A 0xB5026F5AA96619E9ULL | |||
| #define UM 0xFFFFFFFF80000000ULL /* Most significant 33 bits */ | |||
| #define LM 0x7FFFFFFFULL /* Least significant 31 bits */ | |||
| #define LM 0x7FFFFFFFULL /* Least significant 31 bits */ | |||
There was a problem hiding this comment.
We should exclude the files that were copied instead of written for the project. This will just make future merges annoying. I think the list is mt19937-64.c/h, lfz_c, sha256, sha1, strl.c
There was a problem hiding this comment.
And pqsort.c, crccombine.*, crcspeed.*
We can put them in a file called .clang-format-ignore
There was a problem hiding this comment.
Or we can add the do-not-format annotation to the files, marking them from start to end. Then if we have to patch them, we can use multiple annotations so that we still autoformat our patches.
| 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 |
| } else if ((opt[0] == 'g' || opt[0] == 'G') && | ||
| (opt[1] == 'e' || opt[1] == 'E') && | ||
| (opt[2] == 't' || opt[2] == 'T') && opt[3] == '\0' && | ||
| (command_type == COMMAND_SET)) | ||
| { | ||
| } else if ((opt[0] == 'g' || opt[0] == 'G') && (opt[1] == 'e' || opt[1] == 'E') && | ||
| (opt[2] == 't' || opt[2] == 'T') && opt[3] == '\0' && (command_type == COMMAND_SET)) { |
There was a problem hiding this comment.
These are nice to read vertically as "get" vs "GET". I want to disable formatting for this function.
There was a problem hiding this comment.
I'm curious. Does this code really run that much faster than strcasecmp? If it does, can we refactor it into a macro or an inline or something so that it reads something like the following?
} else if (IS_GET(opt) && (command_type == COMMAND_SET))
{
There was a problem hiding this comment.
I don't know but i don't want to spend time on it now. Feel free to improve this if you want.
There was a problem hiding this comment.
The conversion of a char to upper is just one bit flip, c & ~0x20.
|
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>
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.