refactor: Apply the modernize-use-override check with clang-tidy - #3221
refactor: Apply the modernize-use-override check with clang-tidy#3221CryoTheRenegade wants to merge 1 commit into
Conversation
Mark overriding methods with override and drop redundant virtual so MSVC can catch signature mismatches. VC6 still erases override through CppMacros.h. Co-authored-by: Cursor <cursoragent@cursor.com>
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 start a comment with 'qodo' or '@qodo' to chat about any finding |
PR Summary by QodoModernize C++ override declarations across source implementations
AI Description
Diagram
High-Level Assessment
Files changed (80)
|
|
| Filename | Overview |
|---|---|
| Core/GameEngine/Source/GameClient/GUI/IMEManager.cpp | Adds override to the concrete IME manager methods; the signatures match the corresponding virtual interface declarations. |
| Core/Tools/W3DView/ViewerScene.cpp | Marks all local scene-iterator implementations as overrides of matching SceneIterator methods. |
| Core/Tools/W3DView/W3DView.cpp | Marks the standard MFC dialog hooks DoDataExchange and OnInitDialog as overrides. |
| Core/GameEngine/Source/GameNetwork/Network.cpp | Removes redundant virtual specifiers while retaining override, preserving virtual dispatch and signatures. |
| Generals/Code/GameEngine/Source/GameLogic/AI/AI.cpp | Performs the mechanical override-specifier cleanup for the Generals implementation. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/AI/AI.cpp | Mirrors the mechanical override-specifier cleanup for the Zero Hour implementation. |
Reviews (1): Last reviewed commit: "refactor: Apply the modernize-use-overri..." | Re-trigger Greptile
|
The removals appear haphazard as it seems hard to imagine there are only ~850 virtual functions (marked with In my opinion if a function is virtual it should start with the |
yea but this is only in cpp source files, if we wanted to do headers. the functions would incerease to 1100+ header files and 9600+ function definitions. No functions were touched that introduce the virtual to a vtable. So that whittled down the number a bit.
IMO its just redundant as override implies a virtual function and is in line with C++ spec https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#rh-override |
|
Almost all virtual functions are currently marked with
It's a reasonable guideline, but the readability argument is subjective imo. I'd argue the following change makes it harder to see that this function is virtual, not easier: void doFXPos(const Coord3D *primary, const Matrix3D* /*primaryMtx*/, const Real /*primarySpeed*/, const Coord3D * /*secondary*/, const Real /*overrideRadius*/ ) const override |
|
I am also of the mindset of using
instead of
since the former at a glance gives a good idea that a function is virtual and likely inheriting from a sub class. It's much nicer to tell at a glance that a function is virtual. |
Summary
Applies clang-tidy's
modernize-use-overrideto the 846 hits from the modernize inventory. 80.cppfiles, Generals and Zero Hour kept in step.Most of those hits were
virtualon methods that already hadoverride. The check drops the redundantvirtual. The rest were local classes that were still missingoverride, mainlyIMEManager,ViewerScene, andW3DView.Clang-tidy produced the replacements. I filtered out compiler error-recovery edits, checked the diff, and built both games.
Headers are out of this PR. The inventory header filter does not match Windows paths, so the ~9k
virtual ... overridedeclarations in headers are a later pass if we want them.VC6 is fine here.
CppMacros.hturnsoverrideinto nothing on that compiler.