-
Notifications
You must be signed in to change notification settings - Fork 103
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
Improve HELLOIMGUI_WIN32_NO_CONSOLE implementation #99
Comments
Hello, Thanks for your suggestion! Would you want to provide a PR for this, so that you are cited in the contributors? Also, I guess you tried this modification with clang. Can you confirm this, and tell me if you had time to also test it with msvc and/or mingw? |
sure, I will try all your suggestions |
Hi again
I would like to try Mingw, however I have no experience on that. So, is it possible to simply testing the mingw result in your CI pipeline? I have pushed change to my fork: |
You are right, testing on the multiple platform / compilers is one of the main difficult things in this project.
Yes, please submit a PR, and I'll run the pipeline on it. If it does not work on MingW, I'll diagnose it on my side. You did the test I could not do (i.e. with MSVC + clang). Thanks! |
Thanks! |
The mingw pipeline passed. Many thanks for your contribution! |
Version: 0576001
Platform: Win10 + MSVC + clang tool chain
File: hello_imgui_add_app.cmake
Current implantation manually adds link option for MINGW/non-MINGW target which is not necessary, it's also causing a bug for MSVC + clang tool chain as clang could not understand "/subsystem:WINDOWS".
The fix is very simple, CMake supports WIN32 flag in add_executable (https://cmake.org/cmake/help/v3.10/command/add_executable.html)
Just PREPAND the option to app_sources before add_executable.
BTW, it's also possible to APPEND HelloImGui_WinMain.cpp to app_sources before add_excutable so the entire fix could be very clean:
The text was updated successfully, but these errors were encountered: