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
Fixes for MinGW environment. #23
Conversation
The clang test fails on sim_utils.c I didn´t apply clang-format to it because is a third party file and I kept the original formatting. |
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.
Use _WIN32 instead of MINGW to keep the code more portable and compatible with MSVC and Clang
I don't think either of them define MINGW
On the .pro I do use win32-g++, on the sim_utils.c I use MinGW because its a mingw specific issue (which could be on other compilers too) Check this PR or issue As its mingw specific, MINGW32 it is. if we try others in the future, we just add them.
|
@Mihaylov93 yes but the same strsep issue is present on both Clang and MSVC on Windows because of the absence of Linux based functions MinGW also defines _WIN32 The issue isn't compiler specific so much so as, OS specific with strsep being a Linux based function which is absent on Windows on all 3 of the major compiler distributions (and the two major Standard Libs) that target Windows So we could surely do a __MINGW32__ today but tomorrow, when we try to build it on MSVC and Clang, we will end up facing the same issue and so we would have to modify it then So instead of modifying it in the future, let's modify it now |
As strsep is not part of the standard the provider of the string.h could chose to add it or not. The thing is that I am against adding code that I didn't test, I haven't tested clang or MSVC, checking the headers its missing too https://github.com/llvm-mirror/libcxx/blob/master/include/string.h but who knows, its always better to test with the given environments. |
Involves issues #21 #20 #22