-
Notifications
You must be signed in to change notification settings - Fork 67
[GEN][ZH] Add underlying types to enum declarations for GCC build #665
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change adds a lot of new includes. Why is this necessary? Can't we just make one change just with enum types?
Generals/Code/GameEngine/Source/GameNetwork/GameSpy/Thread/BuddyThread.cpp
Outdated
Show resolved
Hide resolved
Generals/Code/GameEngine/Source/GameNetwork/GameSpy/Thread/GameResultsThread.cpp
Outdated
Show resolved
Hide resolved
Generals/Code/GameEngineDevice/Include/W3DDevice/GameClient/WorldHeightMap.h
Outdated
Show resolved
Hide resolved
Can you make a change with just enums types? When I first reviewed it is had no includes. Now it does. Is this necessary? |
Ok, I can do that. I have also included fix to forward declarations, but I can exclude it as well and keep this enum only. |
c5759bc
to
5a6fb7e
Compare
5a6fb7e
to
75c9fe4
Compare
Updated to contain enum forward declaration fixes only. (and required related changes) |
i think we should make proper headers for enums and include where needed instead of making an even bigger mess with the forwards |
This would not compile with GCC though. Forward declare requires type. |
type sizes can in turn be specified there, be it by this macro, it's just out of sight and mind then and would significantly reduce where they are needed given forward declarations are eliminated then |
I understand now. I think what you mean is put the enum definition inside separate files: the enum with its body. And then include that. This is a valid approach, but is probably better done after unification of Generals and Zero Hour is done, because it will create new files and they may be asymmetric because of left over code differences. |
I think a problem with enum files sitting in their own files is longer compile times. Every file requires a new disc access. Forward declaration will be faster. I think the change from zzambers is much simpler and fits right into what EA has provided, without offloading enums into new files. This change is not even adding new forward declarations. It just adds types (with an ugly macro). |
Doing this would be quite significant refactoring effort, requiring lot of human time. (I don't think you can do that using simple script, as easily.) Not even considering, whether avoiding fw enum declarations for price of more header files is actually better. (It is tradeoff and matter of opinion IMO) Also, even if you avoided forward declarations of enums, you will still have lot of other fw declarations (structs, classes) and you cannot easily avoid those due to dependency loops. Either way, I don't think this PR closes doors for any such effort in the future. Actually, I think, even then, it would still be better to do this and get MinGW/GCC CI in GitHub working (and passing) beforehand. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Squash and Merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This knocks out around 7000 W4 warnings in MSVC when building zero hour on it's own.
Looks good to me overall, the only C4471 warnings i see are where forward declarations are missing for enums in view.h, w3dbridgebuffer.h and w3dshadermanager.h
Fixed problems (compilation errors in MinGW/GCC):
This was extracted from bigger change set doing MinGW fixes (see reviews there): #547