Refactor: Isolate WinAPI declarations.#122
Conversation
This commit replaces the direct inclusion of `<windows.h>` with a minimal set of manual forward declarations. This is a deliberate architectural change to: - **Improve Compilation Speed:** Avoids parsing the notoriously large Windows header. - **Eliminate Naming Pollution:** Prevents name clashes with common names like `min`/`max` which conflict with the C++ standard library. - **Enhance Encapsulation:** Makes the library more self-contained by not exposing the entire Windows API.
arun11299
left a comment
There was a problem hiding this comment.
I hope this is well tested and does not cause either windows or Linux builds to fail. I do not have an environment to test this currently.
|
Sorry, I will put this on hold. I am not a fan of declaring system information which is supposed to come from platform include files. I understand it improves the compilation but it is at the cost of added maintenance to keep updating these declarations "in case" things change. Thanks. |
|
Hello, First, thank you for the detailed feedback. I understand and share your concern about the maintenance cost of manually declaring system APIs, because in most situations that would be a significant risk. However, I believe for this specific case (the core Win32 API) this approach is not only safe but also a standard practice in high-quality C++ libraries. On the Stability of the Win32 APIMy main argument is that the core of the Windows C-style API is exceptionally stable. The function signatures and types used in this pull request ( Microsoft goes to extreme lengths to ensure backward compatibility. Changing these fundamental function signatures would break millions of existing applications, which is something they actively avoid at all costs. The risk of these specific declarations becoming outdated is practically zero. Some information about:
In essence, these declarations are as stable as C's The Immediate Benefits for Library UsersThe trade-off for this near-zero maintenance risk is a significant improvement in the library's quality and user experience.
Zero Impact on Non-Windows BuildsAs you noted, platform-specific code should be well-isolated. All these declarations are wrapped in I believe these changes make the library significantly more robust, faster to compile, and easier to use for developers on Windows, without introducing a realistic maintenance burden. Thank you again for considering my contribution. |
|
Thank you for taking time to compile such a detailed response. I am sold. |
|
@ImJotaM Can you please look into the unit tests where it is currently assuming inclusion of windows.h ? The build failed due to use of Sleep function in test_ret_code.cc |
|
The Sleep function is a test-only dependency, so I'll fix this by adding |
This commit replaces the direct inclusion of
<windows.h>with a minimal set of manual forward declarations.This is a deliberate architectural change to:
min/maxwhich conflict with the C++ standard library.