style: apply automated formatting baseline with clang-format#2638
style: apply automated formatting baseline with clang-format#2638DevGeniusCode wants to merge 7 commits into
Conversation
|
| Filename | Overview |
|---|---|
| .clang-format | New formatter configuration introducing Allman brace style, tab indentation, and left-aligned pointers; has three notable gaps: aggregate initializer brace placement (Cpp11BracedListStyle unset), missing AlignConsecutiveDeclarations/Assignments stripping data-block alignment, and Macros expansion injecting spaces inside CPP_11() calls. |
| Core/GameEngine/Source/GameLogic/System/Damage.cpp | Pure whitespace/brace reformatting; notable artifact is the s_bitNameList array closing brace landing on the same line as nullptr (nullptr}) due to unset Cpp11BracedListStyle. |
| Core/GameEngine/Include/Common/GameType.h | Reformatted; loses tab-aligned columns for DEFAULT_WORLD_WIDTH/HEIGHT macros; CPP_11(: Int) gains a spurious space becoming CPP_11( : Int) across all enum declarations in this file. |
| Core/GameEngine/Source/Common/System/GameMemory.cpp | Reformatted; debug-mode constant block loses aligned = signs; #include inside #ifdef now indented by IndentPPDirectives: BeforeHash. |
| Core/GameEngine/Source/Common/System/GameMemoryInitPools_Generals.inl | PoolSizes[] array opening brace moves from Allman to K&R placement due to Cpp11BracedListStyle default; data rows and values unchanged. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[.clang-format baseline] --> B[BraceWrapping: Allman]
A --> C[ColumnLimit: 0]
A --> D[UseTab: AlignWithSpaces]
A --> E[Macros: CPP_11 x =x]
A --> F[AlignConsecutive*: unset]
A --> G[Cpp11BracedListStyle: unset]
B --> B1[✓ class/function/namespace/if braces → Allman]
B --> B2[✗ aggregate/array initializers → K&R via Cpp11BracedListStyle default]
E --> E1[✓ Parses CPP_11 enum macros without tree break]
E --> E2[⚠ Injects space: CPP_11 colon Int → CPP_11 space colon Int]
F --> F1[⚠ Strips column alignment from data blocks]
G --> G1[⚠ Opening brace on same line for list initializers]
G1 --> G2[Closing brace may land on last element line]
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
.clang-format:74-75
**`Macros` expansion injects spaces inside `CPP_11()` call sites**
The `Macros: - CPP_11(x)=x` definition tells clang-format to expand `CPP_11(: Int)` to `: Int` before reformatting. In that expanded context the formatter treats `:` as an enum base-clause token and adds spacing around it, then re-emits the result back inside the parentheses — producing `CPP_11( : Int)` instead of `CPP_11(: Int)` across all call sites in `GameType.h`, `GameCommon.h`, and the broader codebase. This is observable in the diff: every `enum X CPP_11(: Int)` becomes `enum X CPP_11( : Int)`. While it doesn't affect compilation, it is an unintended whitespace change inside macro arguments caused by the Macros config interacting with `SpacesInParentheses: false`.
### Issue 2 of 3
.clang-format:69-70
**`BraceWrapping` does not apply to aggregate/array initializers**
The Allman brace style configured via `BraceWrapping` only covers classes, functions, namespaces, enums, etc. — it does not govern aggregate initializers or array initializers. As a result, declarations like `static PoolSizeRec PoolSizes[] =\n{` (Allman) become `static PoolSizeRec PoolSizes[] = {` (K&R), and the closing `}` in `Damage.cpp` ends up on the same line as the last element (`nullptr}`). This is a known clang-format limitation: aggregate brace placement is controlled by `Cpp11BracedListStyle`, which is unset here and defaults to `true` (K&R for list initializers). If consistent Allman style across all brace contexts is desired, `Cpp11BracedListStyle: false` would be needed.
### Issue 3 of 3
.clang-format:38-39
**Formatter removes column alignment in data-oriented blocks**
Neither `AlignConsecutiveDeclarations` nor `AlignConsecutiveAssignments` are configured, so both default to `None`. This causes the formatter to strip the intentional column alignment from data-oriented blocks across the codebase. For example, `GameType.h` loses the aligned spacing between `DEFAULT_WORLD_WIDTH`/`DEFAULT_WORLD_HEIGHT` and their values; `AsciiString.h` loses alignment between the two `unsigned short` member declarations; `GameMemory.cpp` loses the aligned `=` signs in its debug constant block. The team's rule requires maintaining column alignment in data blocks and lookup tables — the current config actively removes it.
Reviews (6): Last reviewed commit: "style: fix whitespace in typedefs and it..." | Re-trigger Greptile
a9d279e to
9ddb58a
Compare
|
There are still a few formatting improvements I haven’t addressed in this pass, such as consecutive assignment alignment. Current (Before): AudioAffect_Music = 0x01,
AudioAffect_Sound = 0x02,
AudioAffect_Sound3D = 0x04,
AudioAffect_All = (Music | Sound | Sound3D),Target (After): AudioAffect_Music = 0x01,
AudioAffect_Sound = 0x02,
AudioAffect_Sound3D = 0x04,
AudioAffect_All = (Music | Sound | Sound3D),If there is interest, I can implement it |
Core\GameEngine)Core\GameEngine)
This is good. |
Core\GameEngine)|
Updated |
This comment was marked as resolved.
This comment was marked as resolved.
xezon
left a comment
There was a problem hiding this comment.
Looks very promising again. I like the minimal formatting on this first pass. Just a few more things to polish.
| if you define MEMORYPOOL_INTENSE_VERIFY, we do intensive verifications after | ||
| nearly every memory operation. this is OFF by default, since it slows down | ||
| things a lot, but is worth turning on for really obscure memory corruption issues. | ||
| */ |
There was a problem hiding this comment.
The block macros are indented with 2 spaces instead of tabs. In the github page they will show with different indent width.
There was a problem hiding this comment.
We can try this:
-UseTab: AlignWithSpaces
+UseTab: ForIndentationChatty: #### Why this works:
- Preprocessor Comments: File-scope comments nested inside preprocessor directives are formatted using spaces only across header, body, and footer. This prevents the tab-space mismatch on GitHub.
- Standard Code: Standard nested code blocks still indent using tabs.
- Parameter Lists: Parameters wrapped across multiple lines align using spaces only, preventing them from expanding horizontally.
| */ | ||
| * Closes the current file if it is open. | ||
| * Must call LocalFile::close() for each successful LocalFile::open() call. | ||
| */ |
There was a problem hiding this comment.
Also strange that here the block comment has only 1 leading space.
There was a problem hiding this comment.
This behavior seems hardcoded in clang-format.
There was a problem hiding this comment.
Is it a bug that triggers it? I would have expected that it applies uniform indentation always.
There was a problem hiding this comment.
I researched with chatty, and it seems to be a feature instead of a bug. According to the tool, these are all 3 different concepts:
Explanation of the Comment Indentation (Starred vs. Unstarred comments)
For anyone reviewing the diff and wondering about the varying indentation of block comments (specifically why some closing tags have */ with a space and others have */ without a space), this is actually expected behavior from clang-format. It is not a bug in the configuration.
Here is what is happening under the hood:
1. The Three Block Comment Styles in the Repository
Our codebase currently contains three different styles of comment blocks, which clang-format handles differently:
- Style A: Single-Starred (
*) (e.g.,LocalFile.cpp):/** * Closes the current file if it is open. */
- Style B: Double-Starred (
**) (e.g.,dx8polygonrenderer.h):/** ** DX8PolygonRendererClass ** This is a record of a batch/range of polygons to be rendered. */
- Style C: Unstarred / Plain text (e.g.,
LocalFile.cpp):/** Allocate a buffer large enough to hold entire file, read the entire file into the buffer, then close the file. */
2. How clang-format Processes These Styles
- Single-Starred (
*):clang-formatrecognizes this as standard Javadoc/Doxygen notation. It aligns the body stars vertically under the first asterisk of the opening/**tag. Since/**starts at Column 0, the body stars and the closing tag align at Column 1, resulting in exactly 1 leading space before the closing tag (*/). - Double-Starred (
**):clang-formatdoes not recognize**as standard Javadoc notation. It treats it as custom text. To avoid breaking it, it aligns the body lines flush at Column 0 (0 leading spaces) and keeps the closing*/at Column 0. - Unstarred (Plain): Because there are no stars at all, it wraps and indents the body text at 2 spaces relative to the start of the block, but aligns the closing
*/flush at Column 0.
This is why we see a mix of */ (1 space) and */ (0 spaces) in the diff—the tool is simply preserving the different styles present in the original files.
3. The Hardcoded Logic inside LLVM
Inside the LLVM codebase for clang-format (specifically in the source file BreakableComment.cpp), the formatter processes block comments using the BreakableBlockComment class:
- It checks if the comment is a "decorated" comment (whether the body lines start with
*). - If they do, it automatically calculates the alignment column of the asterisks.
- The alignment target column is hardcoded to the index of the first asterisk of the opening tag (index 1 of
/*or/**). - The formatter forces every subsequent line—including the closing
*/—to align its asterisk at Column 1.
Because this logic is hardcoded inside the formatter's parsing engine, there are no configuration options in .clang-format to override this (e.g., there is no option like AlignClosingBlockComment: false).
4. LLVM Community Context & Backlash
This is a known limitation that many developers have run into:
- The Consensus: Teams have repeatedly requested a flag to customize block comment star-alignment (specifically to allow the closing
*/to have 0 spaces of indentation instead of 1 space). - The LLVM Maintainers' Position: The LLVM project leads have historically rejected adding configuration options for this. Their reasoning is that comments are treated as raw text and reflowed under the
ColumnLimit, and adding options to customize every variant of comment ASCII art (such as box comments, Javadoc headers, and ending alignments) would over-complicate and bloat the configuration schema. - The Backlash: As a result, developers who want custom block comment layouts are forced to choose between:
- Accepting
clang-format's hardcoded 1-space indentation (*/) for all starred comments. - Using
// clang-format off/// clang-format onguards around their comment blocks. (author note: I don't like the concept of adding clang-format off in the code) - Disabling comment formatting entirely via
ReflowComments: false(which removes the valuable auto-wrapping of comments). - Running an automated Python script pass: Running a one-time script across the repository to standardize all block comments into a single uniform style (e.g., converting all double-starred and unstarred comments into standard single-starred Javadoc comments) prior to running the formatter. (While this cleans up the existing codebase, it is a one-time fix and does not prevent future PRs or newer comments from introducing out-of-style comments that will drift again.)
- Accepting
14366de to
e19a3a9
Compare
|
Updated |
e19a3a9 to
e191a7c
Compare
Prerequisites: Please Merge First
This PR is the final step of the formatting baseline. It depends on two preliminary structural PRs that must be merged first to ensure a clean, 100% automated formatting pass without breaking VC6 compilation:
>>merging in C++98.Objective
This PR introduces a minimalist, non-destructive
.clang-formatbaseline. To make this easy to review and discuss, I have scoped the formatting only to the.\Core\GameEnginedirectory.The "Do No Harm" Approach
The primary goal of this configuration is to avoid the "destructive" behavior of standard formatters.
ColumnLimit: 0: Automatic line wrapping is completely disabled. Clang-format will respect our manual line breaks and will not pack or split arguments unexpectedly.What is being formatted?
To establish a consistent baseline, the formatter enforces a few core rules:
Why this change was made -now-:
clang-formatfundamentally requires a brace style to function; it cannot simply "ignore" braces even on a pure whitespace pass. Since running the tool was inevitably going to touch braces and affect line counts anyway, I decided to "go all in" and establish a fully consistent style across the board, rather than trying to fight the tool to maintain an inconsistent legacy mix.void* ptr).AlignTrailingComments: false) with a fixed 5-space offset. This prevents "diff pollution" (the ripple effect where adding one long function name forces all surrounding comments to shift).\) are left-aligned (AlignEscapedNewlines: DontAlign) with a single space. This eliminates the "mixed whitespace" anti-pattern and ensures macros render perfectly straight in GitHub and all IDEs, regardless of the user's tab width setting.Legacy / VC6 Compatibility
A lot of effort went into making sure this doesn't break our legacy support:
Standard: c++20to correctly parse modern syntax while maintaining VC6 compatibility structurally.AttributeMacrosandMacros) to safely parse ourCPP_11enum macros without breaking the formatting tree.AllowShortBlocksOnASingleLine: Alwaysso macros and empty stubs aren't vertically bloated.Future Phases (Minimizing the Initial Diff)
Please note that several settings in this baseline (such as
SortIncludes: false,AllowShortFunctionsOnASingleLine: All, andAllowShortBlocksOnASingleLine: Always) are intentionally included purely to minimize the initial diff and avoid overwhelming changes.