Skip to content

Fix Windows header include order#77

Merged
DanielaOrtner merged 1 commit into
RebelToolbox:mainfrom
madmiraal:fix-windows-includes
Jun 14, 2024
Merged

Fix Windows header include order#77
DanielaOrtner merged 1 commit into
RebelToolbox:mainfrom
madmiraal:fix-windows-includes

Conversation

@madmiraal
Copy link
Copy Markdown
Contributor

Header files should not depend on the order that they are included. Specifically, header files should not depend on other header files being included first. Unfortunately, on Windows, the order that Windows system header files are included is important. For example, winsock2.h will raise a warning if it is not included before windows.h:

winsock2.h|15|warning: #warning Please include winsock2.h before windows.h

The problem is, even if we include winsock2.h before windows.h in the header file where winsock2.h is included, it doesn't guarantee that windows.h has not been included by another header file earlier. The solution is not to include winsock2.h before every time we include windows.h, because winsock2.h may not be needed there. Furthermore, this problem is not limited to winsock2.h. For example, in e557849, mmsystem.h needed to be included after windows.h.

The solution requires understanding why winsock2.h needs to be included before windows.h, and mmsystem.h needs to be included after windows.h. windows.h is often included when developing on Windows, because it defines many macros that Windows code uses. Unfortunately, presumably to make it simpler for beginner developers, windows.h also includes a number of other Windows system header files; even if they are not required or wanted. One of these system header files is winsock.h and another is mmsystem.h. winsock2.h wants to be included before winsock.h. mmsystem.h only wants to be included if it's not included with windows.h.

To avoid, winsock.h, mmsystem.h and other unnecessary Windows system headers being included whenever windows.h is included, WIN32_LEAN_AND_MEAN can be defined before windows.h is included for the first time. To ensure WIN32_LEAN_AND_MEAN is defined the first time windows.h is included, it must be defined before every time windows.h is included. Unfortunately, since many other Windows system header files may also include windows.h, we need to define WIN32_LEAN_AND_MEAN before every Windows system header file is included. Furthermore, since Rebel's coding style (#19) ensures that header files are included in alphabetical order, which would place windows.h below most other system header files, we need to ensure that enforcing Rebel's code style does not place other Windows system header files before #define WIN32_LEAN_AND_MEAN and #include <windows.h>.

This PR ensures that:

  • #define WIN32_LEAN_AND_MEAN is placed before every #include <windows.h>
  • If other Windows system files are included:
    • #define WIN32_LEAN_AND_MEAN and #include <windows.h> are placed before other Windows system files
    • A comment (// Windows system includes come after <windows.h>) is placed before other Windows system files to break the include block and the alphabetic header file ordering, and explains the deliberate change from alphabetic ordering of includes.

@madmiraal madmiraal added the PR Type: Bug Fix Your current game should now work as expected. label Jun 14, 2024
@DanielaOrtner DanielaOrtner self-requested a review June 14, 2024 11:23
Copy link
Copy Markdown
Contributor

@DanielaOrtner DanielaOrtner left a comment

Choose a reason for hiding this comment

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

Great work!

@DanielaOrtner DanielaOrtner merged commit 34bc483 into RebelToolbox:main Jun 14, 2024
@madmiraal madmiraal deleted the fix-windows-includes branch June 14, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR Type: Bug Fix Your current game should now work as expected.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants