Skip to content

refactor: Add override keyword to virtual function overrides in Tools and set compile option -Wsuggest-override#2394

Open
bobtista wants to merge 12 commits intoTheSuperHackers:mainfrom
bobtista:bobtista/override-keyword-tools
Open

refactor: Add override keyword to virtual function overrides in Tools and set compile option -Wsuggest-override#2394
bobtista wants to merge 12 commits intoTheSuperHackers:mainfrom
bobtista:bobtista/override-keyword-tools

Conversation

@bobtista
Copy link

@bobtista bobtista commented Mar 3, 2026

Summary

Context

Part 6/6 of splitting #2101. Depends on #2389 merging first.

Notes

  • 201 files changed
  • All lines retain the virtual keyword
  • The -Wsuggest-override compiler warning ensures future virtual overrides use the keyword

@greptile-apps
Copy link

greptile-apps bot commented Mar 3, 2026

Greptile Summary

This PR is the final installment (Part 6/6) of a large refactoring series that adds the override keyword to virtual function overrides across the Tools (WorldBuilder, GUIEdit, wdump) in both Generals and GeneralsMD, and enables the -Wsuggest-override GCC/Clang warning to enforce it going forward. It also adds the missing virtual keyword to ParticleSystemManagerDummy overrides in Core (a catch from #2392).\n\nKey changes:\n- 201 header and source files across Generals/, GeneralsMD/, and Core/ receive override annotations on virtual function overrides — no logic is changed.\n- cmake/compilers.cmake adds -Wsuggest-override for non-MSVC, non-VS6 toolchains so any future override missing the keyword will be flagged by the compiler.\n- PlaceholderView::isZoomLimited() in both wbview3d.cpp files gains const alongside override — this is a silent correctness fix, since View::isZoomLimited() is declared const in the base class and the original stub was inadvertently creating a separate (non-overriding) function that shadowed the base class virtual.\n- Four methods in PlaceholderView (isDoingScriptedCamera, stopDoingScriptedCamera, lookAt, initHeightForMap) are still missing override in both wbview3d.cpp files despite having matching signatures in the base View class. With -Wsuggest-override now active, these will produce compiler warnings on GCC/Clang builds.

Confidence Score: 4/5

Safe to merge; only style refactoring with one minor gap that will emit compiler warnings from the newly added flag.

The PR is a mechanical refactor with no logic changes. The sole substantive issue is that four PlaceholderView overrides in both wbview3d.cpp files are missing override, which will generate -Wsuggest-override warnings from the flag this very PR introduces. That inconsistency is worth resolving in this PR, but it does not affect runtime behaviour or build correctness.

Generals/Code/Tools/WorldBuilder/src/wbview3d.cpp and GeneralsMD/Code/Tools/WorldBuilder/src/wbview3d.cpp — four PlaceholderView overrides are still missing the override keyword.

Important Files Changed

Filename Overview
cmake/compilers.cmake Adds -Wsuggest-override for non-MSVC non-VS6 compilers in the correct branch; MSVC equivalent deferred to a future PR per previous review discussion.
Core/GameEngine/Include/GameClient/ParticleSys.h Adds virtual to ParticleSystemManagerDummy override declarations to match codebase conventions; was missed in the prior PR (#2392).
Generals/Code/Tools/WorldBuilder/src/wbview3d.cpp Adds override to most PlaceholderView overrides; also correctly adds const to isZoomLimited to match View::isZoomLimited() const; four pure/virtual overrides (isDoingScriptedCamera, stopDoingScriptedCamera, lookAt, initHeightForMap) are missing override and will trigger the newly enabled -Wsuggest-override warning.
GeneralsMD/Code/Tools/WorldBuilder/src/wbview3d.cpp Same as Generals counterpart; override correctly added to most PlaceholderView overrides, but isDoingScriptedCamera, stopDoingScriptedCamera, lookAt, and initHeightForMap are still missing override.
Generals/Code/Tools/GUIEdit/Include/GUIEditDisplay.h Adds override to all GUIEditDisplay virtual function overrides; setBorderShroudLevel gains both virtual and override for consistency.
GeneralsMD/Code/Tools/wdump/wdump.cpp Adds override to CWDumpCommandLineInfo::ParseParam and CAboutDlg::DoDataExchange; straightforward.
Generals/Code/Tools/WorldBuilder/src/WorldBuilderDoc.cpp Adds override to three local OutputStream subclass write() overrides; no logic changes.
Generals/Code/Tools/WorldBuilder/src/WorldBuilder.cpp Adds override to WBGameFileClass::Set_Name, WB_W3DFileSystem::Get_File, and CAboutDlg::DoDataExchange; no logic changes.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[cmake/compilers.cmake] -->|adds -Wsuggest-override| B{non-MSVC, non-VS6?}
    B -->|Yes| C[GCC / Clang warns on missing override]
    B -->|No| D[MSVC: no change — deferred to future PR]
    C --> E[201 header/source files]
    E --> F[override added to matching virtual overrides]
    E --> G[isZoomLimited gains const — fixes silent shadow]
    E --> H[ParticleSystemManagerDummy gains virtual keyword]
    F --> I{4 functions in PlaceholderView}
    I -->|isDoingScriptedCamera / stopDoingScriptedCamera\nlookAt / initHeightForMap| J[⚠️ override still missing — will trigger warning]
Loading
Prompt To Fix All With AI
This is a comment left during a code review.
Path: Generals/Code/Tools/WorldBuilder/src/wbview3d.cpp
Line: 199-203

Comment:
**Missing `override` will trigger `-Wsuggest-override` warnings**

This PR enables `-Wsuggest-override` in `cmake/compilers.cmake`, but `PlaceholderView` in this file still has several functions that override base class virtuals without the `override` keyword. These will produce warnings on GCC/Clang builds:

- `isDoingScriptedCamera()` — pure virtual in `View`
- `stopDoingScriptedCamera()` — pure virtual in `View`
- `lookAt(const Coord3D *o)` — virtual in `View`
- `initHeightForMap(void)` — virtual in `View`

The same four functions in `GeneralsMD/Code/Tools/WorldBuilder/src/wbview3d.cpp` (lines 200–204) have the same omission.

Note: `scrollBy(Coord2D *delta)` and `resetCamera`/`rotateCamera`/etc. are intentionally left without `override` because their signatures differ from the base class (missing the `easeIn`/`easeOut` default parameters or a missing `const` qualifier), so `override` would cause a compile error there.

```suggestion
	virtual Bool isDoingScriptedCamera() override { return false; }
	virtual void stopDoingScriptedCamera() override {}

	virtual void lookAt( const Coord3D *o ) override {};
	virtual void initHeightForMap( void ) override {};
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (8): Last reviewed commit: "fix: Add missing virtual keyword to Part..." | Re-trigger Greptile

@xezon xezon added the Refactor Edits the code with insignificant behavior changes, is never user facing label Mar 10, 2026
@bobtista bobtista force-pushed the bobtista/override-keyword-tools branch from 1630354 to b7df0dc Compare March 11, 2026 01:48
@Skyaero42
Copy link

Might want to do a quick spacing check before I review?

@bobtista
Copy link
Author

Might want to do a quick spacing check before I review?

Looks good - found some mixed tab/space stuff for indentation but the override {} spacing seems good

Copy link

@Skyaero42 Skyaero42 left a comment

Choose a reason for hiding this comment

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

This looks good

@xezon xezon changed the title refactor(Tools): Add override keyword and -Wsuggest-override warning refactor(Tools): Add override keyword to virtual function overrides in Tools and set compile option -Wsuggest-override Mar 17, 2026
@xezon xezon changed the title refactor(Tools): Add override keyword to virtual function overrides in Tools and set compile option -Wsuggest-override refactor: Add override keyword to virtual function overrides in Tools and set compile option -Wsuggest-override Mar 17, 2026
@bobtista bobtista force-pushed the bobtista/override-keyword-tools branch from ee24d63 to 58990fc Compare March 26, 2026 19:10
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.

3 participants