Skip to content

style: Apply clang-format to entire codebase - #2820

Open
mirelle7 wants to merge 1 commit into
TheSuperHackers:mainfrom
mirelle7:chore/clang-format-codebase
Open

mirelle7 wants to merge 1 commit into
TheSuperHackers:mainfrom
mirelle7:chore/clang-format-codebase

Conversation

@mirelle7

@mirelle7 mirelle7 commented Jun 21, 2026 •

Copy link
Copy Markdown

This PR applies a repository-wide mechanical reformatting across all project source code using clang-format 22.1.8.

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-revs file containing the commit hash of this formatting pass so that git 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: AlignWithSpaces
+UseTab: ForIndentation
+SpacesInAngles: Leave
+Cpp11BracedListStyle: false
  • 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: ForIndentation GameLogic.h 25–45

Current (Before):

#pragma once

#include "Common/GameCommon.h"	// ensure we get DUMP_PERF_STATS, or not
#include "Common/GameType.h"
#include "Common/Snapshot.h"
#include "Common/STLTypedefs.h"
#include "Common/ObjectStatusTypes.h"
#include "GameNetwork/NetworkDefs.h"
#include "GameLogic/AI.h"
#include "GameLogic/Module/UpdateModule.h"	// needed for DIRECT_UPDATEMODULE_ACCESS
#pragma once

#include "Common/GameCommon.h"    // ensure we get DUMP_PERF_STATS, or not
#include "Common/GameType.h"
#include "Common/Snapshot.h"
#include "Common/STLTypedefs.h"
#include "Common/ObjectStatusTypes.h"
#include "GameNetwork/NetworkDefs.h"
#include "GameLogic/AI.h"
#include "GameLogic/Module/UpdateModule.h"    // needed for DIRECT_UPDATEMODULE_ACCESS

2. SpacesInAngles: Leave PerfTimer.cpp 205–225

/*static*/ Bool AutoPerfGatherIgnore::s_ignoring = false;

//-------------------------------------------------------------------------------------------------
typedef std::vector< std::pair< AsciiString, AsciiString >/**/> StringPairVec;
/*static*/ Bool AutoPerfGatherIgnore::s_ignoring = false;

//-------------------------------------------------------------------------------------------------
typedef std::vector< std::pair< AsciiString, AsciiString > /**/> StringPairVec;

3. Cpp11BracedListStyle: false WOLGameSetupMenu.cpp 218–226

static GameWindow *genericPingWindow[MAX_SLOTS] = {0};

static const Image *pingImages[3] = { nullptr, nullptr, nullptr };
static GameWindow* genericPingWindow[MAX_SLOTS] = { 0 };

static const Image* pingImages[3] = { nullptr, nullptr, nullptr };

All other open review items from PR #2638 have been resolved, are hardcoded clang-format parser behaviors (such as the Doxygen Javadoc star alignment), or have since landed on main.


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:

  1. VC6 Template Compatibility (SpacesInAngles: Leave):
    Prevents clang-format from 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.
  2. VC6 Braced Initializer Compatibility (Cpp11BracedListStyle: false):
    Prevents formatting braced-init-lists in C++11 style, which causes parsing issues on legacy compilers.
  3. MSVC Inline Assembly Safety:
    clang-format has 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 { ... }) in main via PR chore: Prevent conflict between clang-format and MSVC by wrapping inline assembly blocks in curly braces #2811, making the formatter pass entirely safe.
  4. Macro-Split Assignments Resolved:
    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.

  • Open to Review Rounds: This PR is open to further review rounds. If any formatting settings need adjustments, the codebase can be formatted again and the branch force-pushed.
  • Trivial Merge Conflict Resolution: If another pull request gets merged into main before this one, resolving conflicts is trivial: we simply re-run the formatter locally on top of the updated main branch 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 main without checking out this branch:

# 1. Fetch the PR branch head natively from the repository URL
Write-Host "Fetching PR branch..."
git fetch https://github.com/TheSuperHackers/GeneralsGameCode.git pull/2820/head

# 2. Pull the .clang-format configuration from the PR commit
Write-Host "Getting .clang-format configuration..."
git checkout FETCH_HEAD -- .clang-format

# 3. Format the codebase locally
Write-Host "Formatting codebase locally..."
Get-ChildItem -Path Core, Generals, GeneralsMD, scripts, resources/gitinfo -Include *.cpp, *.h, *.inl -Recurse | ForEach-Object { clang-format -i $_.FullName }

# 4. Compare your local formatted working tree directly against the PR branch in-memory
# (We add --ignore-cr-at-eol to handle Windows CRLF line ending differences cleanly)
Write-Host "Comparing local formatting against the PR..."
git diff --ignore-cr-at-eol FETCH_HEAD -- Core Generals GeneralsMD scripts resources/gitinfo

Expected result: The diff output 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.

git fetch upstream pull/2820/head:clang-fmt
git rebase clang-fmt -Xignore-all-space

@DevGeniusCode

Copy link
Copy Markdown
-AlignEscapedNewlines: Left

If I remember correctly, we intentionally chose left alignment with a 4-character spacing.

@mirelle7

Copy link
Copy Markdown
Author
-AlignEscapedNewlines: Left

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.

@xezon

xezon commented Jun 22, 2026

Copy link
Copy Markdown

What is the correct strategy to migrate old branches to new main after the formatting was merged into main?

@mirelle7

mirelle7 commented Jun 22, 2026 •

Copy link
Copy Markdown
Author

removed this text, everything is in the main description

@xezon

xezon commented Jun 22, 2026

Copy link
Copy Markdown

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?

@DevGeniusCode

Copy link
Copy Markdown
-UseTab: AlignWithSpaces
+UseTab: ForIndentation
+SpacesInAngles: Leave
+Cpp11BracedListStyle: false

Can you plz add short code examples before and after the changes?

@mirelle7
mirelle7 marked this pull request as draft June 23, 2026 12:49
@mirelle7

Copy link
Copy Markdown
Author

I'll update the description soon with more validating, newer guide, and code examples.

@xezon

xezon commented Jun 29, 2026

Copy link
Copy Markdown

This needs progress.

@mirelle7
mirelle7 force-pushed the chore/clang-format-codebase branch 2 times, most recently from bf5acb4 to 0a067f8 Compare July 6, 2026 17:32
@mirelle7

mirelle7 commented Jul 6, 2026

Copy link
Copy Markdown
Author
image

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

@mirelle7
mirelle7 marked this pull request as ready for review July 6, 2026 17:46
@xezon

xezon commented Jul 6, 2026

Copy link
Copy Markdown

So now the big question is: WHEN MERGE? 👀

: m_offset(0)
, m_size(0)
: m_offset(0)
, m_size(0)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leading tab and then 2 spaces, is that as intended? Naturally we would have expected leading tabs.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread Core/GameEngine/Include/Common/Debug.h Outdated
#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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a clang format off would be better readable for these macros

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does not sound ideal for this one case.

@mirelle7 mirelle7 Sep 13, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about:
#define DEBUG_LOG(m) ((void)(DebugLog m, 0)) // Log message with trailing new line character (LF)
#define DEBUG_LOG_RAW(m) ((void)(DebugLogRaw m, 0)) // Log message without trailing new line character (LF)
#define DEBUG_LOG_LEVEL(l, m) ((void)((l & DebugLevelMask) ? (DebugLog m, 0) : 0))
#define DEBUG_LOG_LEVEL_RAW(l, m) ((void)((l & DebugLevelMask) ? (DebugLogRaw m, 0) : 0))
#define DEBUG_ASSERTLOG(c, m) ((void)((!(c)) ? (DebugLog m, 0) : 0))

Quickly checking I found 16 of these instances in this codebase. 10 of them can be fixed in this way. The remaining 6 are complex enough where the expansion would make sense.

Comment thread Core/GameEngine/Include/Common/INI.h Resolved
@xezon xezon added the Refactor Edits the code with insignificant behavior changes, is never user facing label Jul 7, 2026
@mirelle7
mirelle7 force-pushed the chore/clang-format-codebase branch from 0a067f8 to cd0af70 Compare July 17, 2026 21:44
@xezon

xezon commented Jul 19, 2026

Copy link
Copy Markdown

Dependencies #2883, #2885 merged

@mirelle7
mirelle7 force-pushed the chore/clang-format-codebase branch from cd0af70 to d65ade8 Compare July 20, 2026 21:23
@mirelle7
mirelle7 force-pushed the chore/clang-format-codebase branch from d65ade8 to f1e1de1 Compare July 29, 2026 12:32
@mirelle7

Copy link
Copy Markdown
Author

Regenerated with:

IndentPPDirectives: None
IndentCaseLabels: false
BraceWrapping:
AfterEnum: false
AllowShortEnumsOnASingleLine: true
AllowShortCaseLabelsOnASingleLine: true

@xezon xezon left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks almost very good

@mirelle7
mirelle7 force-pushed the chore/clang-format-codebase branch from f1e1de1 to c8524e9 Compare July 30, 2026 14:48
@mirelle7

Copy link
Copy Markdown
Author

Regenerated with:
BraceWrapping: AfterEnum: true
AllowShortEnumsOnASingleLine: false

@mirelle7
mirelle7 force-pushed the chore/clang-format-codebase branch 3 times, most recently from be9637b to 77f0cd5 Compare July 31, 2026 22:18
@mirelle7
mirelle7 force-pushed the chore/clang-format-codebase branch from 659a3e9 to 1347200 Compare September 13, 2026 19:56
@xezon

xezon commented Sep 14, 2026

Copy link
Copy Markdown

Needs rebase and update.

@mirelle7
mirelle7 force-pushed the chore/clang-format-codebase branch from 1347200 to 4e88810 Compare September 14, 2026 14:25
for (int i = 0; i < STATECOUNT; ++i)
{

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every single line loop or condition now adds the {}. Is this intentional? It will add a extra vertical space when not always needed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InsertBraces: true did that. Reverting on next regeneration.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For starters I would be happy if braces are only added if it is missing on one of the if else branches.

So for example

if (...)
{
 // does something
}
else
 // does something else without braces

to

if (...)
{
 // does something
}
else
{
 // does something else
}

@mirelle7
mirelle7 force-pushed the chore/clang-format-codebase branch from 4e88810 to e8a547f Compare September 20, 2026 17:29
@TheSuperHackers TheSuperHackers deleted a comment from coderabbitai Bot Sep 20, 2026
@xezon

xezon commented Sep 20, 2026

Copy link
Copy Markdown

Were the last comments addressed?

@TheSuperHackers TheSuperHackers deleted a comment from greptile-apps Bot Sep 21, 2026
@mirelle7

Copy link
Copy Markdown
Author

this was a small regression fix. I'll look into the last comments soon

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Walkthrough

Warning

Review details and warnings were omitted to fit the comment limit.

@greptile-apps

greptile-apps Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[Low impact] The formatting pass reaches source code across the repository without intending to change runtime behavior.

The table-alignment requirement needs to be satisfied before merging.

Findings

  1. P2 Table columns no longer align ▶

Summary

Adds .clang-format and applies a repository-wide C++ formatting pass.

  • C++ source files now share one formatting style.

Reviews (4) · Last reviewed commit: "chore: Apply updated clang-format rules ..." · Reviewed by Greptile

@xezon xezon changed the title chore: Apply clang-format to entire codebase style: Apply clang-format to entire codebase Sep 27, 2026
Comment thread Core/Tools/matchbot/wlib/xtime.cpp Resolved
@xezon

xezon commented Oct 4, 2026

Copy link
Copy Markdown

Can be rebased.

@mirelle7
mirelle7 force-pushed the chore/clang-format-codebase branch from c9d4779 to dbff175 Compare October 8, 2026 14:19
mirelle7 added a commit to mirelle7/GeneralsGameCode that referenced this pull request Oct 14, 2026
…sm block layout

Follow-up to TheSuperHackers#2811, prerequisite for the clang-format pass in TheSuperHackers#2820.

clang-format joins consecutive brace-less `__asm` statements onto one
line, and MSVC treats everything after a brace-less `__asm` up to the end
of the line as assembly, which pulls following C++ statements into the
asm (e.g. `__asm fistp ... return retval;`). Wrap the remaining
occurrences in `__asm { }` blocks, and inline the multi-line TRANSPOSE
asm macro into its only use.

clang-format also never formats the inside of an inline assembly block,
and formats `_asm` followed by `{` on the next line as a continuation.
Write every block as `__asm {` on one line with a tab-indented body, asm
labels at the `__asm` level and no `;` after the closing brace.

VC6 codegen of all affected translation units is unchanged.
@CryoTheRenegade

Copy link
Copy Markdown

would adding a preset like BasedOnStyle: WebKit be beneficial for all of the smaller options within clang-format?

@mirelle7
mirelle7 force-pushed the chore/clang-format-codebase branch from dbff175 to 8f77e35 Compare October 26, 2026 16:07
Comment on lines +135 to +138
{ "Filename", INI::parseAsciiString, nullptr, offsetof(AudioEventInfo, m_filename) },
{ "Volume", INI::parsePercentToReal, nullptr, offsetof(AudioEventInfo, m_volume) },
{ "VolumeShift", INI::parsePercentToReal, nullptr, offsetof(AudioEventInfo, m_volumeShift) },
{ "MinVolume", INI::parsePercentToReal, nullptr, offsetof(AudioEventInfo, m_minVolume) },

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Table columns no longer align

The formatter removes the aligned columns from AudioEventInfo::m_audioEventInfo, making its rows harder to scan. The repository requires columns in changed lookup tables and initialization lists to stay aligned. Restore this alignment before merging and adjust the formatting settings so future runs preserve it. The same pattern appears in other tables across the PR.

Rule Used: Align columns in data-oriented blocks (like lookup tables, initialization lists, or structured data) for readability; otherwise prefer single spacing. When modifying a block, maintain alignment only within that specific block being changed. (source)

Learned From
TheSuperHackers/GeneralsGameCode#2067

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the old version don't look aligned to me as well

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You’re right that the old formatting isn’t especially clear in the diff view. Looking at the source, though, the initializer fields were padded with tabs so the columns lined up in an editor; the new version replaces that with single spaces, so the columns no longer line up. My comment should have made that distinction clearer.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think its fine. The padding also has a cost for diffs, because adding a new row with columns wider than all others in a block forces updating all rows, which is not good.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That’s a fair trade-off: padding can make a new wider row touch every existing row, while the formatter’s single-spacing avoids that churn. This is a style/readability choice, not a correctness issue, so I’ll withdraw the finding for this PR. The current review guidance prefers aligned columns in data-oriented blocks; should I update it to prefer single spacing when alignment would cause table-wide edits for new rows?

@mirelle7

Copy link
Copy Markdown
Author

I checked out all the webkit settings and didn't find any useful changes. Most things looked more wrong after this.

@CryoTheRenegade

Copy link
Copy Markdown

I checked out all the webkit settings and didn't find any useful changes. Most things looked more wrong after this.

Fair, i was just using it as an example.

@xezon

xezon commented Oct 27, 2026

Copy link
Copy Markdown

Was there anything left to tweak here or is this ready for re-review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactor Edits the code with insignificant behavior changes, is never user facing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add clang format to project

7 participants