chore: Apply clang-format to entire codebase - #2820
Conversation
|
Too many files changed for review (3000 files, 500 file limit). |
If I remember correctly, we intentionally chose left alignment with a 4-character spacing. |
Thanks. Updated description, accidentally pasted older version. This line wasn't changed. |
|
What is the correct strategy to migrate old branches to new main after the formatting was merged into main? |
|
removed this text, everything is in the main description |
|
Did you test it? Does it work? This does not look as if it would work. I would expect the formatting needs to be merged into each commit of the branch? |
Can you plz add short code examples before and after the changes? |
|
I'll update the description soon with more validating, newer guide, and code examples. |
|
This needs progress. |
bf5acb4 to
0a067f8
Compare
Updated PRs description. Code examples added. Tested the script from "Migrate Existing Feature Branches" between a clang-formatted main from 2026-07-06 and 10 prs. Undrafted. Tested against 10 open PRs: the single rebase command resolves all whitespace-only conflicts automatically. Only genuine code-logic overlaps require manual resolution (1–6 hunks in the affected PRs). Per-commit formatting is not required |
|
So now the big question is: WHEN MERGE? 👀 |
| : m_offset(0) | ||
| , m_size(0) | ||
| : m_offset(0) | ||
| , m_size(0) |
There was a problem hiding this comment.
Leading tab and then 2 spaces, is that as intended? Naturally we would have expected leading tabs.
There was a problem hiding this comment.
This was intentional to prevent Github from showing the wrong width. We can revert this to "AlignWithSpaces".
"UseTab: ForIndentation: Fixes the tab-space rendering mismatch in preprocessor block comments on GitHub (where GitHub defaults to tab=8, causing space-aligned block comments inside tab-indented preprocessor blocks to look misaligned)."
Didn't find a UseTab that wouldn't either uglify, keep github aligned or uses a single type of whitespace.
| #define DEBUG_LOG_RAW(m) do { { DebugLogRaw m ; } } while (0) // Log message without trailing new line character (LF) | ||
| #define DEBUG_LOG_LEVEL(l, m) do { if (l & DebugLevelMask) { DebugLog m ; } } while (0) | ||
| #define DEBUG_LOG_LEVEL_RAW(l, m) do { if (l & DebugLevelMask) { DebugLogRaw m ; } } while (0) | ||
| #define DEBUG_ASSERTLOG(c, m) do { { if (!(c)) DebugLog m ; } } while (0) |
There was a problem hiding this comment.
Maybe a clang format off would be better readable for these macros
There was a problem hiding this comment.
We can also try AfterControlStatement: MultiLine.
"Changing AfterControlStatement from Always to MultiLine tells clang-format to only break the { onto a new line if the controlling expression spans multiple lines. Since do has no condition, and single-line if (cond) conditions stay on one line, the { stays inline for all short macro bodies."
There was a problem hiding this comment.
That does not sound ideal for this one case.
xezon
added
the
Refactor
0a067f8 to
cd0af70
Compare
cd0af70 to
d65ade8
Compare
d65ade8 to
f1e1de1
Compare
|
Regenerated with: IndentPPDirectives: None |
f1e1de1 to
c8524e9
Compare
|
Regenerated with: |
ebf1d78 to
be9637b
Compare
77f0cd5 to
d698faf
Compare
There was a problem hiding this comment.
Overall, looks good.
I would recommend ripping of the bandaid a.s.a.p.
We also need to have checks in place that all new code follows clang format (or that the author has applied clang format).
If I understand correctly (with a bit of help from Claude), you can add the following to CI:
- name: Check formatting (diff-only)
run: |
git fetch origin ${{ github.base_ref }} --depth=1
git-clang-format-18 --binary clang-format-18 --diff origin/${{ github.base_ref }} HEADor if git-clang-format isn't available
- name: Check formatting
run: |
git fetch origin ${{ github.base_ref }} --depth=1
git diff --name-only origin/${{ github.base_ref }}...HEAD -- '*.cpp' '*.h' \
| xargs -r clang-format --dry-run --Werror@tintinhamans what are our options here.
d698faf to
105648a
Compare
PR Summary by QodoEstablish repository-wide clang-format baseline
AI Description
Diagram
High-Level Assessment
Files changed (21)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can turn on the rule miner and Qodo learns your standards from review history |
|
As for accidental behavior changes, I compared two VS22 release builds, one with this PR and one without. I disabled I also checked with retail CRC and SAVE compatibility disabled and same result there. |
|
Switch cases still use indentation. switch (m_type)
{
case CONSTANT:
DEBUG_ASSERTLOG(m_low == m_high, ("m_low != m_high for a CONSTANT GameLogicRandomVariable"));
if (m_low == m_high)
{
return m_low;
}
FALLTHROUGH;
case UNIFORM:
return GameLogicRandomValueReal(m_low, m_high);
default:
/// @todo fill in support for nonuniform GameLogicRandomVariables.
DEBUG_CRASH(("unsupported DistributionType in GameLogicRandomVariable::getValue"));
return 0.0f;
}Expected: switch (m_type)
{
case CONSTANT:
DEBUG_ASSERTLOG(m_low == m_high, ("m_low != m_high for a CONSTANT GameLogicRandomVariable"));
if (m_low == m_high)
{
return m_low;
}
FALLTHROUGH;
case UNIFORM:
return GameLogicRandomValueReal(m_low, m_high);
default:
/// @todo fill in support for nonuniform GameLogicRandomVariables.
DEBUG_CRASH(("unsupported DistributionType in GameLogicRandomVariable::getValue"));
return 0.0f;
} |
| "TerrainRoadType", | ||
| 64, | ||
| 64, | ||
| }, |
There was a problem hiding this comment.
There are a couple more cases like this in that file and the Zero Hour version.
| //virtual void* getDevice() override { return nullptr; } | ||
| // virtual void openDevice() override {} | ||
| // virtual void closeDevice() override {} | ||
| // virtual void* getDevice() override { return nullptr; } |
There was a problem hiding this comment.
I suspect it cannot differentiate between code that's commented out and regular comments, right?
5 participants
This PR applies a repository-wide mechanical reformatting across all project source code using
clang-format.This is the successor to the original formatting attempt in PR #2638 (submitted by @DevGeniusCode). Following up on the community poll in Discord ("Apply clang-format on all code", which passed with 53%), this PR establishes the formatting baseline. Because the output is 100% automated, if conflicts arise from other PRs landing before this one, regenerating the diff is trivial (re-run the formatter and force-push).
A follow-up PR will add a
.git-blame-ignore-revsfile containing the commit hash of this formatting pass so thatgit blame(both locally and on GitHub) automatically skips over the formatting changes and preserves the historical authorship of every line.What Changed vs PR #2638
Three config adjustments have been applied based on review feedback and legacy toolchain compatibility:
UseTab: ForIndentation: Fixes the tab-space rendering mismatch in preprocessor block comments on GitHub (where GitHub defaults to tab=8, causing space-aligned block comments inside tab-indented preprocessor blocks to look misaligned).SpacesInAngles: Leave: Retains existing spacing inside template brackets (e.g., keeping> >rather than collapsing them to>>), which is required for legacy C++98 / VC6 compiler compatibility.Cpp11BracedListStyle: false: Prevents the formatter from collapsing braced initializer lists to C++11 single-line style, maintaining Allman brace layout consistency.1.
UseTab: ForIndentationGameLogic.h 25–45Current (Before):
2.
SpacesInAngles: LeavePerfTimer.cpp 205–2253.
Cpp11BracedListStyle: falseWOLGameSetupMenu.cpp 218–226All other open review items from PR #2638 have been resolved, are hardcoded
clang-formatparser behaviors (such as the Doxygen Javadoc star alignment), or have since landed onmain.Key Legacy & Toolchain Compatibility Settings
To ensure the codebase continues to compile cleanly on both modern toolchains (VS2022) and the legacy target toolchain (Visual C++ 6), the following settings are established in
.clang-format:SpacesInAngles: Leave):Prevents
clang-formatfrom collapsing nested template arguments (e.g.,std::vector<std::vector<int> >to>>). VC6 and older compilers misparse>>as a right-shift operator. PR chore: Prevent conflict between clang-format and pre-C++11 nested template parsing #2760 (and its closed predecessor PR refactor: Extract nested templates into typedefs for legacy compatibility #2642) already resolved the instances where it was collapsed, and this setting ensures it stays that way.Cpp11BracedListStyle: false):Prevents formatting braced-init-lists in C++11 style, which causes parsing issues on legacy compilers.
clang-formathas known parser conflicts with MSVC-style inline assembly blocks when not enclosed in curly braces (which can cause the formatter to join assembly lines and break compilation). These have been pre-emptively wrapped in curly braces (__asm { ... }) inmainvia PR chore: Prevent conflict between clang-format and MSVC by wrapping inline assembly blocks in curly braces #2811, making the formatter pass entirely safe.Problems regarding assignment formatting split across macros have been pre-emptively addressed by PR refactor: Eliminate macro-split assignments #2641.
Scope & Merging Strategy
The PR includes the full codebase diff across all source files.
mainbefore this one, resolving conflicts is trivial: we simply re-run the formatter locally on top of the updatedmainbranch and force-push.Zero-Trust Verification
Reviewers can verify that this PR is 100% mechanically generated with no manual edits by running the following steps directly from
mainwithout checking out this branch:Expected result: The
diffoutput must be completely empty. If silent, it guarantees the PR contains only tool-generated formatting changes.How to Migrate Existing Feature Branches
If you have an active feature branch developed before this bulk formatting pass, you can migrate it to the new formatted main branch by letting Git automatically ignore whitespace-only conflicts during the rebase. Any remaining conflicts after this step are genuine code-logic overlaps between your changes and this PR and will need to be resolved manually as usual.