Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Conversation

Mihaylov93
Copy link
Collaborator

Involves issues #21 #20 #22

@Mihaylov93
Copy link
Collaborator Author

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.

Copy link
Collaborator

@pratikpc pratikpc left a 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

@Mihaylov93
Copy link
Collaborator Author

Mihaylov93 commented Oct 4, 2020

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.

#ifdef __clang__
/*code specific to clang compiler*/
#elif __GNUC__
/*code for GNU C compiler */
#elif _MSC_VER
/*usually has the version number in _MSC_VER*/
/*code specific to MSVC compiler*/
#elif __BORLANDC__
/*code specific to borland compilers*/
#elif __MINGW32__
/*code specific to mingw compilers*/
#endif

@pratikpc
Copy link
Collaborator

pratikpc commented Oct 4, 2020

@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

@Mihaylov93
Copy link
Collaborator Author

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.

@guerinoni guerinoni closed this Oct 5, 2020
@Mihaylov93 Mihaylov93 changed the title Remove Build_XX folder and implement fixes for MinGW environment. Fixes for MinGW environment. Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants