style: Convert leading spaces to tabs in Core and Dependencies#2561
style: Convert leading spaces to tabs in Core and Dependencies#2561bobtista wants to merge 10 commits intoTheSuperHackers:mainfrom
Conversation
|
| Filename | Overview |
|---|---|
| scripts/cpp/convert_leading_spaces_to_tabs.py | New script documenting how whitespace was converted; top_dirs still includes Dependencies/Utility even though the HEAD commit reverted those files as incompatible with the script |
| Core/GameEngine/Source/GameNetwork/GameInfo.cpp | Whitespace-only: leading spaces converted to tabs; no logic changes |
| Core/GameEngineDevice/Source/W3DDevice/GameClient/W3DView.cpp | Whitespace-only: leading spaces converted to tabs, including previously mixed tab+space lines; no logic changes |
| Core/GameEngine/Source/Common/System/GameMemory.cpp | Whitespace-only: corrects mixed space+tab indentation to pure tabs; no logic changes |
| Core/Libraries/Source/WWVegas/WWLib/cpudetect.cpp | Whitespace-only: GNU __asm__ call-style lines correctly converted; inline asm brace blocks preserved as-is |
| Core/GameEngine/Include/Common/INI.h | Whitespace-only: private member declarations and one public method corrected from spaces to tabs |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Read C++ file\ncp1252 encoding] --> B[Preprocess: expand\nconfusing macros]
B --> C[tree-sitter parse\nC++ AST]
C --> D{Too many\nparse errors?}
D -->|Yes| E[Skip file]
D -->|No| F[Process each line]
F --> G{In block comment\nor ASM block?}
G -->|Yes| H[Preserve as-is]
G -->|No| I{Preprocessor\ndirective?}
I -->|Yes| J[Preserve, track pp_depth]
I -->|No| K{Already tabs /\ncontinuation line?}
K -->|Yes| H
K -->|No| L[Query AST node\nat line start col]
L --> M[get_indent_depth:\ncount scope ancestors]
M --> N{depth==0 and\nspaces>=4 or pp_depth>0?}
N -->|Yes - sanity skip| H
N -->|No| O[Replace leading\nwhitespace with N tabs]
O --> P[Write converted\nfile back]
Prompt To Fix All With AI
This is a comment left during a code review.
Path: scripts/cpp/convert_leading_spaces_to_tabs.py
Line: 443-454
Comment:
**`top_dirs` still includes `Dependencies/Utility`**
The HEAD commit (`a9cbcc22`) explicitly reverted the seven `Dependencies/Utility/Utility/*.h` files because the script "cannot reliably handle the interaction between preprocessor nesting and C++ nesting in these files." However, `top_dirs` still lists that directory, so re-running the script as committed would re-process — and likely corrupt — those same files.
```suggestion
top_dirs = [
os.path.join(root_dir, 'Core'),
os.path.join(root_dir, 'Generals'),
os.path.join(root_dir, 'GeneralsMD'),
]
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (5): Last reviewed commit: "style: Exclude Dependencies/Utility comp..." | Re-trigger Greptile
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
By what rules are these whitespaces edited? How can it be reviewed?
| //using the IMC_SETCOMPOSITIONWINDOW message. | ||
| // | ||
| //I'm not sure what to do here. | ||
| m_result = 1; |
Leading whitespace is regenerated from the C++ AST, not counted from spaces. For more details on how it's done, check scripts/cpp/convert_leading_spaces_to_tabs.py it will:
It skips a bunch of things eg block comments, and things that are addressed in later PRs to make review easier. To review it, I'd make sure you like the approach in the script, then spot check the results. |
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
There are still some indentation errors.
|
|
||
| const Coord3D *pos = obj->getPosition(); | ||
| Real angle = obj->getOrientation(); | ||
| Real angle = obj->getOrientation(); |
| Int cy = REAL_TO_INT_FLOOR((y + 0.5f)/PATHFIND_CELL_SIZE_F); | ||
| if (cx >= 0 && cy >= 0 && cx < m_extent.hi.x && cy < m_extent.hi.y) | ||
| { | ||
| for (Int iy = 0; iy < numStepsY; ++iy, tl_x += ydx, tl_y += ydy) |
| } | ||
| } | ||
|
|
||
| return ; |
There was a problem hiding this comment.
Side note: I thought we had all the superfluous returns already removed.
There was a problem hiding this comment.
looks like the space between return and semicolon was never considered.
There was a problem hiding this comment.
This return is at the end of the function and can be removed. We already had such a change previously, but perhaps it missed W3DView.
There was a problem hiding this comment.
I'm aware of that. I was merely making a note that the space between return and the semicolon, which the original script/regexp may not have considered.
There was a problem hiding this comment.
The original script is only focused on leading spaces. Removing the space between the return and semicolon would be a separate PR, as would removing the return
There was a problem hiding this comment.
Ah I see it was started but never finished: #1811
I can pick that up.
Core/Tools/W3DView/BoneMgrDialog.cpp
Outdated
| @@ -525,8 +525,8 @@ BoneMgrDialogClass::Remove_Object_From_Bone | |||
| { | |||
| // Loop through all the children of this bone | |||
| for (HTREEITEM hchild_item = m_BoneTree.GetChildItem (bone_item); | |||
| (hchild_item != nullptr); | |||
| hchild_item = m_BoneTree.GetNextSiblingItem (hchild_item)) { | |||
| (hchild_item != nullptr); | |||
There was a problem hiding this comment.
Can we put one more indent after for, while, if, else if?
Skyaero42
left a comment
Skyaero42
left a comment
There was a problem hiding this comment.
Sampled a few files. Comments are recurring throughout the PR.
| Bool m_use3DSoundRangeVolumeFade; // TheSuperHackers @feature Enables 3D sound range volume fade as originally intended | ||
| Real m_3DSoundRangeVolumeFadeExponent; // TheSuperHackers @feature Sets 3D sound range volume fade exponent for non-linear fade. | ||
| // The higher the exponent, the sharper the decline at the max range. | ||
| // The higher the exponent, the sharper the decline at the max range. |
There was a problem hiding this comment.
Incorrect indentation, now it looks like this comment belongs to the next line, rather than the previous
| mov esi,[buf] | ||
| mov ecx,[len] | ||
| dec ecx | ||
| mov edi,[crcPtr] |
There was a problem hiding this comment.
Incorrect indentation. Multiple times. This function is now a mess
| * memory pointed at by buffer. Returns the number of bytes read. | ||
| * Returns -1 if an error occurred. | ||
| */ | ||
| * memory pointed at by buffer. Returns the number of bytes read. |
There was a problem hiding this comment.
Documentation block comment should be placed before function - multiple times in this is file
| Int winActivate(); ///< pop window to top of list and activate | ||
| Int winBringToTop(); ///< bring this window to the top of the win list | ||
| Int winEnable( Bool enable ); /**< enable/disable a window, a disbled | ||
| window can be seen but accepts no input */ |
There was a problem hiding this comment.
Place documentation comment before function
| public: | ||
|
|
||
| virtual VideoStreamInterface* next() override; ///< Returns next open stream | ||
| virtual VideoStreamInterface* next() override; ///< Returns next open stream |
Improve conversion script to handle: - Inline assembly blocks (skip entirely) - Tabs-then-spaces alignment (preserve as-is) - Mixed space-before-tab indentation (use tab count) - Mid-line block comment opens (track for next line) Manually fix tree-sitter depth errors in AIPathfind.cpp and TerrainVisual.cpp caused by heavy preprocessor conditionals.
DevGeniusCode
left a comment
DevGeniusCode
left a comment
There was a problem hiding this comment.
I manually reviewed some of the files (started from the bottom of the list and worked my way up). I left comments on the main indentation issues I noticed.
| #if __has_builtin(__builtin_return_address) | ||
| static inline uintptr_t _ReturnAddress() | ||
| { | ||
| return reinterpret_cast<uintptr_t>(__builtin_return_address(0)); | ||
| return reinterpret_cast<uintptr_t>(__builtin_return_address(0)); | ||
| } | ||
| #else | ||
| #error "No implementation for _ReturnAddress" |
There was a problem hiding this comment.
The script seems to have flattened the indentation within this preprocessor block. The function body should be indented further than the function signature.
| } | ||
|
|
||
| return wsOut; | ||
| } | ||
|
|
||
| inline char* WINAPI ConvertBSTRToString(BSTR pSrc) | ||
| { | ||
| DWORD cb, cwch; | ||
| char *szOut = nullptr; | ||
|
|
||
| if (!pSrc) | ||
| return nullptr; | ||
|
|
||
| // Retrieve the size of the BSTR with the null terminator | ||
| cwch = SysStringLen(pSrc) + 1; | ||
|
|
||
| // Compute the needed size with the null terminator | ||
| cb = WideCharToMultiByte(CP_ACP, 0, pSrc, cwch, nullptr, 0, nullptr, nullptr); | ||
| if (cb == 0) | ||
| { | ||
| cwch = GetLastError(); | ||
| _com_issue_error(!IS_ERROR(cwch) ? HRESULT_FROM_WIN32(cwch) : cwch); | ||
| return nullptr; | ||
| } | ||
|
|
||
| // Allocate the string | ||
| szOut = (char*)::operator new(cb * sizeof(char)); | ||
| if (!szOut) | ||
| { | ||
| _com_issue_error(HRESULT_FROM_WIN32(ERROR_OUTOFMEMORY)); | ||
| return nullptr; | ||
| } | ||
|
|
||
| // Convert the string and null-terminate | ||
| szOut[cb - 1] = '\0'; | ||
| if (WideCharToMultiByte(CP_ACP, 0, pSrc, cwch, szOut, cb, nullptr, nullptr) == 0) | ||
| { | ||
| // We failed, clean everything up | ||
| cwch = GetLastError(); | ||
|
|
||
| ::operator delete(szOut); | ||
| szOut = nullptr; | ||
|
|
||
| _com_issue_error(!IS_ERROR(cwch) ? HRESULT_FROM_WIN32(cwch) : cwch); | ||
| } | ||
|
|
||
| return szOut; | ||
| DWORD cb, cwch; | ||
| char *szOut = nullptr; | ||
|
|
||
| if (!pSrc) | ||
| return nullptr; |
There was a problem hiding this comment.
The script seems to be adding unnecessary padding here. The function body is misaligned, and the indentation level is no longer consistent with the rest of the code.
| inline void Matrix3::Get_Z_Vector(Vector3 * set) const | ||
| { | ||
| set->Set(Row[0][2], Row[1][2], Row[2][2]); | ||
| set->Set(Row[0][2], Row[1][2], Row[2][2]); | ||
| } |
There was a problem hiding this comment.
While the script works correctly in simple cases like this one, it remains inconsistent overall, leading to the broken indentation seen in more complex blocks (like preprocessor directives).
| (m_stringBackgroundBMP.CompareNoCase (pszBackgroundBMP) != 0)) | ||
| { | ||
| { | ||
| // Create a new instance of the BMP object to use |
| if (m_pCBackgroundBMP) | ||
| { | ||
| // Remove the background BMP from the scene | ||
| // and release its pointer | ||
| m_pCBackgroundBMP->Remove (); |
| { | ||
| ASSERT (m_pC2DScene); | ||
| if (m_pC2DScene) | ||
| { | ||
| // Remove the old background BMP if there was one. |
| if ((use_global_reset_flag && m_bAutoCameraReset) || | ||
| ((use_global_reset_flag == false) && allow_reset) || | ||
| m_bOneTimeReset) { | ||
| pCGraphicView->Reset_Camera_To_Display_Object (*m_pCRenderObj); | ||
| m_bOneTimeReset = false; | ||
| } |
| // Update the animation frame | ||
| if (m_pCRenderObj) | ||
| { | ||
| // Update the animation frame | ||
| for (int i = 0; i < m_pCAnimCombo->Get_Num_Anims(); i++) |
These files use space-based indentation inside #if blocks for platform-specific code. The script cannot reliably handle the interaction between preprocessor nesting and C++ nesting in these files, so they are excluded from conversion.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| top_dirs = [ | ||
| os.path.join(root_dir, 'Core'), | ||
| os.path.join(root_dir, 'Generals'), | ||
| os.path.join(root_dir, 'GeneralsMD'), | ||
| os.path.join(root_dir, 'Dependencies', 'Utility'), | ||
| ] | ||
| file_list = [] | ||
| for ext in ['*.cpp', '*.h', '*.inl']: | ||
| for top_dir in top_dirs: | ||
| file_list.extend( | ||
| glob.glob(os.path.join(top_dir, '**', ext), recursive=True) | ||
| ) |
There was a problem hiding this comment.
top_dirs still includes Dependencies/Utility
The HEAD commit (a9cbcc22) explicitly reverted the seven Dependencies/Utility/Utility/*.h files because the script "cannot reliably handle the interaction between preprocessor nesting and C++ nesting in these files." However, top_dirs still lists that directory, so re-running the script as committed would re-process — and likely corrupt — those same files.
| top_dirs = [ | |
| os.path.join(root_dir, 'Core'), | |
| os.path.join(root_dir, 'Generals'), | |
| os.path.join(root_dir, 'GeneralsMD'), | |
| os.path.join(root_dir, 'Dependencies', 'Utility'), | |
| ] | |
| file_list = [] | |
| for ext in ['*.cpp', '*.h', '*.inl']: | |
| for top_dir in top_dirs: | |
| file_list.extend( | |
| glob.glob(os.path.join(top_dir, '**', ext), recursive=True) | |
| ) | |
| top_dirs = [ | |
| os.path.join(root_dir, 'Core'), | |
| os.path.join(root_dir, 'Generals'), | |
| os.path.join(root_dir, 'GeneralsMD'), | |
| ] |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/cpp/convert_leading_spaces_to_tabs.py
Line: 443-454
Comment:
**`top_dirs` still includes `Dependencies/Utility`**
The HEAD commit (`a9cbcc22`) explicitly reverted the seven `Dependencies/Utility/Utility/*.h` files because the script "cannot reliably handle the interaction between preprocessor nesting and C++ nesting in these files." However, `top_dirs` still lists that directory, so re-running the script as committed would re-process — and likely corrupt — those same files.
```suggestion
top_dirs = [
os.path.join(root_dir, 'Core'),
os.path.join(root_dir, 'Generals'),
os.path.join(root_dir, 'GeneralsMD'),
]
```
How can I resolve this? If you propose a fix, please make it concise.4 participants
Summary
git diff -wis empty)Access specifiers (
public:/private:/protected:) are placed at the classbrace level, matching the existing codebase convention.
Script
The formatting script is included in the PR for reference but is not intended
to be merged — it shows how the changes were generated.
See
scripts/cpp/convert_leading_spaces_to_tabs.pyfor details.Part 1 of 4 — Core/ and Dependencies/ (499 files).
See also: #2562, #2563, #2564.