Skip to content

Fix multiline brace placement for clang#3540

Open
madolson wants to merge 1 commit intovalkey-io:unstablefrom
madolson:fix-multiline-brace-placement
Open

Fix multiline brace placement for clang#3540
madolson wants to merge 1 commit intovalkey-io:unstablefrom
madolson:fix-multiline-brace-placement

Conversation

@madolson
Copy link
Copy Markdown
Member

@madolson madolson commented Apr 20, 2026

So when we merged the original clang format, we merged something I think a few of us had an issue with which was indentation on multi-line control statements made it hard to see when if ended and when the code began, example:

            if (!rioWriteBulkCount(r, '*', 2 + cmd_items) || !rioWriteBulkString(r, "RPUSH", 5) ||
                !rioWriteBulkObject(r, key)) {
                listTypeReleaseIterator(li);
                return 0;
            }

listTypeReleaseIterator is not part of the conditional, but it looks like it is. The current code formatter forces this. Without a column limit, there is not way to force the multi-line, but the provided configuration makes it "allowed".

            if (!rioWriteBulkCount(r, '*', 2 + cmd_items) || !rioWriteBulkString(r, "RPUSH", 5) ||
                !rioWriteBulkObject(r, key))
            {
                listTypeReleaseIterator(li);
                return 0;
            }

Also, the original Redis style did this.

…atements

Change BreakBeforeBraces from Attach to Custom with
AfterControlStatement: MultiLine. This moves the opening brace of
multiline if/for/while/else-if conditions to its own line, improving
readability by visually separating the condition from the body.

Before:
    if (foo &&
        bar) {
        body;
    }

After:
    if (foo &&
        bar)
    {
        body;
    }

This matches the style used by Redis for multiline control statements.

All source files reformatted with clang-format-18 using the updated
config (51 files, 324 instances).

Signed-off-by: Madelyn Olson <madelyneolson@gmail.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 20, 2026

Codecov Report

❌ Patch coverage is 83.66534% with 41 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.41%. Comparing base (0327c27) to head (899a63d).

Files with missing lines Patch % Lines
src/sentinel.c 0.00% 18 Missing ⚠️
src/module.c 12.50% 7 Missing ⚠️
src/valkey-cli.c 66.66% 4 Missing ⚠️
src/server.c 85.71% 3 Missing ⚠️
src/config.c 66.66% 2 Missing ⚠️
src/replication.c 90.90% 2 Missing ⚠️
src/blocked.c 50.00% 1 Missing ⚠️
src/cluster_legacy.c 96.66% 1 Missing ⚠️
src/cluster_migrateslots.c 90.90% 1 Missing ⚠️
src/fuzzer_command_generator.c 83.33% 1 Missing ⚠️
... and 1 more
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #3540      +/-   ##
============================================
- Coverage     76.41%   76.41%   -0.01%     
============================================
  Files           159      159              
  Lines         79927    79925       -2     
============================================
- Hits          61078    61072       -6     
- Misses        18849    18853       +4     
Files with missing lines Coverage Δ
src/acl.c 92.52% <100.00%> (ø)
src/anet.c 72.44% <ø> (ø)
src/aof.c 80.37% <100.00%> (+0.06%) ⬆️
src/bitops.c 96.22% <100.00%> (ø)
src/childinfo.c 97.33% <100.00%> (ø)
src/cli_common.c 64.90% <100.00%> (ø)
src/cluster.c 92.45% <100.00%> (+0.23%) ⬆️
src/cluster_slot_stats.c 93.75% <100.00%> (ø)
src/commandlog.c 95.79% <100.00%> (ø)
src/db.c 94.50% <100.00%> (ø)
... and 41 more

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zuiderkwast
Copy link
Copy Markdown
Contributor

I prefer that we don't merge this change. We have already taken the hit and removed that style. Changing back now two years later will cause merge conflicts again.

Arguably, placing the brace on its own line or not depending on the condition in the if/while is also a bit unintuitive. If you're not aware of the rule, you'd just think that it's random and that both styles are accepted (brace on same line or on its own line).

@madolson
Copy link
Copy Markdown
Member Author

madolson commented Apr 21, 2026

We can backport it to avoid the merge conflicts, everything will be consistent across branches.

I don't know what to say about it being unintuitive. The alternative of forcing indentation is also weird. It was the style consistently used in redis (and still is).

@zuiderkwast
Copy link
Copy Markdown
Contributor

We can backport it to avoid the merge conflicts, everything will be consistent across branches.

We have lots of development forks and work going on now. Forkless, key blocking, sync replication, raft cluster, ... lot's of merge conflicts to handle.

The alternative of forcing indentation is also weird. It was the style consistently used in redis (and still is).

We have this style with the opening brace always at the end of the line (subjectively weird or not) consistency used in Valkey for two years... and you think we should copy Redis?

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.

3 participants