Modernize clang-format: enforce 120-column limit and reduce config to meanful overrides by harrylin98 · Pull Request #2 · harrylin98/valkey_forked

harrylin98 · 2026-05-26T21:56:53Z

This is internal draft PR:

This PR changes the .clang-format configuration and reformats src/* accordingly.

Reformat: The second commit applies the new config across all src/*.{c,h} (122 files). This is a mechanical reformatting with no logic changes.

Config changes:

ColumnLimit: 0 → ColumnLimit: 120: The old config disabled all line length enforcement. The new limit prevents unbounded line growth, with specific penalty break rules.

 PenaltyReturnTypeOnItsOwnLine: 1000000 // always keep return type on same line as function name

 PenaltyBreakAssignment: 100 // prefer not breaking over splitting assignments

 PenaltyBreakOpenParenthesis: 1000 // avoid breaking right after '('

Removed 16 redundant options: These are already set by LLVM's default.
BinPackArguments: false: one argument per line when they don't all fit.
AllowAllArgumentsOnNextLine: false: prevents dumping all args on the next line.
ReflowComments: false: don't reflow comment text and preserves intentional formatting.
SkipMacroDefinitionBody: true: don't reformat macro bodies.
AllowShortEnumsOnASingleLine: false: one enum value per line.
BreakBeforeBinaryOperators: NonAssignment: break before ||, &&.

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

JimB123

I added a few comments/notes. I just scanned files up to cluster_legacy.

Overall, I think many of the changes are neutral. Some of them are really good. A few are seem negative.

If I had to guess regarding TSC feedback, I think they'll probably nitpick the things that they don't like, and dismiss the rest as mostly neutral.

For me, I think that overall it's an improvement.

JimB123 · 2026-05-27T16:17:39Z

+                                                                    "dataset changes will be retained.",
+                                                                    0,
+                                                                    abortCommandHandler,
+                                                                    lua),


This section isn't great. Between the expansion to multi-lines, and the shifting due to string size, I think this might generate push back.

JimB123 · 2026-05-27T16:23:16Z

+    size_t *commands_len) {
    VALKEYMODULE_NOT_USED(module_ctx);


This blends the function contents into the parameters.

Personally, I can live with this, but I'd rather see something like:

static int fn ( int p1, int p2) { // indented 8 to avoid blending with code // code begins (indented 4)

Can we try ContinuationIndentWidth set at 8? Will this fix this?

JimB123 · 2026-05-27T16:26:43Z

-    case LUA_TSTRING:
-        ValkeyModule_ReplyWithStringBuffer(ctx, lua_tostring(lua, -1), lua_strlen(lua, -1));
-        break;
+    case LUA_TSTRING: ValkeyModule_ReplyWithStringBuffer(ctx, lua_tostring(lua, -1), lua_strlen(lua, -1)); break;


I think this one is a negative. I like the original better.

Maybe if ALL of the case branches could fit like this, but it's inconsistent.

JimB123 · 2026-05-27T16:32:14Z

+    if (ValkeyModule_CreateCommand(ctx, "hellocluster.pingall", PingallCommand_ValkeyCommand, "readonly", 0, 0, 0)
+        == VALKEYMODULE_ERR)
        return VALKEYMODULE_ERR;


This is similar to my comment about function parameters. It bends the IF continuation with the next line.

Ideally, I'd do this

Suggested change

if (ValkeyModule_CreateCommand(ctx, "hellocluster.pingall", PingallCommand_ValkeyCommand, "readonly", 0, 0, 0)

== VALKEYMODULE_ERR)

return VALKEYMODULE_ERR;

if (ValkeyModule_CreateCommand(ctx, "hellocluster.pingall", PingallCommand_ValkeyCommand, "readonly", 0, 0, 0)

== VALKEYMODULE_ERR)

return VALKEYMODULE_ERR;

JimB123 · 2026-05-27T16:37:18Z

+        case ACL_DENIED_DB:
+            le->object = argpos ? sdsdup(objectGetVal(c->argv[argpos])) : sdsdup(c->cmd->fullname);
+            break;


This case is inconsistent, but I personally like the change. I think the original line was just getting too large/complex. IMO this change helps readability.

JimB123 · 2026-05-27T16:39:05Z

+#define AE_BARRIER  4 /* With WRITABLE, never fire the event if the                                                     \
+                         READABLE event already fired in the same event                                                 \
+                         loop iteration. Useful when you want to persist                                                \
+                         things to disk before sending replies, and want                                                \


This change was unexpected. Why did this occur?

JimB123 · 2026-05-27T16:46:57Z

+    if (rep->type != VALKEYMODULE_REPLY_STRING && rep->type != VALKEYMODULE_REPLY_SIMPLE_STRING
+        && rep->type != VALKEYMODULE_REPLY_ERROR)


This one seems worse.

JimB123 · 2026-05-27T17:16:36Z

+    {"client-tls-port",
+     auxAnnounceClientTlsPortSetter,
+     auxAnnounceClientTlsPortGetter,
+     auxAnnounceClientTlsPortPresent},


This change seems worse.

JimB123 · 2026-05-27T17:18:23Z

+            if (!(flags & (CLUSTER_NODE_FAIL | CLUSTER_NODE_PFAIL)) && nodeInNormalState(node) && node->ping_sent == 0
+                && clusterNodeFailureReportsCount(node) == 0) {


This seems worse. Looking at the original, why wasn't that refactored earlier?

harrylin98 added 2 commits May 20, 2026 14:40

Optimize Clang format configs

83938f4

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

Reformat code using new clang configs

aed50c0

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

github-actions Bot assigned harrylin98 May 26, 2026

JimB123 reviewed Jun 2, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize clang-format: enforce 120-column limit and reduce config to meanful overrides#2

Modernize clang-format: enforce 120-column limit and reduce config to meanful overrides#2
harrylin98 wants to merge 2 commits into
unstablefrom
clang_format

harrylin98 commented May 26, 2026 •

edited

Loading

Uh oh!

JimB123 left a comment

Uh oh!

JimB123 May 27, 2026

Uh oh!

JimB123 May 27, 2026

Uh oh!

JimB123 Jun 2, 2026

Uh oh!

JimB123 May 27, 2026

Uh oh!

JimB123 May 27, 2026

Uh oh!

JimB123 May 27, 2026

Uh oh!

JimB123 May 27, 2026

Uh oh!

JimB123 May 27, 2026

Uh oh!

JimB123 May 27, 2026

Uh oh!

JimB123 May 27, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if (rep->type != VALKEYMODULE_REPLY_STRING && rep->type != VALKEYMODULE_REPLY_SIMPLE_STRING
		&& rep->type != VALKEYMODULE_REPLY_ERROR)

		if (!(flags & (CLUSTER_NODE_FAIL \| CLUSTER_NODE_PFAIL)) && nodeInNormalState(node) && node->ping_sent == 0
		&& clusterNodeFailureReportsCount(node) == 0) {

Conversation

harrylin98 commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

This is internal draft PR:

This PR changes the .clang-format configuration and reformats src/* accordingly.

Reformat: The second commit applies the new config across all src/*.{c,h} (122 files). This is a mechanical reformatting with no logic changes.

Config changes:

Uh oh!

JimB123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

harrylin98 commented May 26, 2026 •

edited

Loading