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

fix MSVC build #12

Closed
wants to merge 1 commit into from
Closed

fix MSVC build #12

wants to merge 1 commit into from

Conversation

kostyaorkostya
Copy link
Contributor

see https://msdn.microsoft.com/en-us/library/a3140177.aspx to read about MSVC intrinsics

@aklomp
Copy link
Owner

aklomp commented Jun 29, 2016

Looks good, but instead of checking for __WIN32__ and friends, couldn't you explicitly check for MSVC? Maybe GCC on Win32 also defines __WIN32__.

@BurningEnlightenment
Copy link
Contributor

Looks good, but instead of checking for __WIN32__ and friends, couldn't you explicitly check for MSVC? Maybe GCC on Win32 also defines __WIN32__.

You're right; this is a MSVC specific fix and enabling it if _WIN32 or __WIN32__ are defined will break gcc and clang builds on windows. An appropriate preprocessor check would be:

#if defined(_MSC_VER)

@kostyaorkostya
Copy link
Contributor Author

Yep, I'll fix this, was a little busy

@BurningEnlightenment
Copy link
Contributor

BurningEnlightenment commented Jul 2, 2016

Errr, I'm very curious how you came across this... those intrinsics are only used in (enc|dec)/uint(32|64).c and because Visual Studio doesn't define __WORDSIZE at all, none of those files should have been included. I just noticed that, because I compiled the test executable using my cmake build files and visual studio without applying your proposed fix. To my very amusement it compiled just fine and the tests passed without any errors 😄.
However, I'll merge @yazevnul fix into #7 and correct the preprocessor checks over there.
EDIT: done

@aklomp
Copy link
Owner

aklomp commented Jul 4, 2016

Just pushed a fixed version to Master. I amended the original commit, so authorship is properly attributed.

@aklomp aklomp closed this Jul 4, 2016
@BurningEnlightenment
Copy link
Contributor

@aklomp you haven't fixed the #if __WORDSIZE == * in codec_plain.c 😞, whithout this MSVC won't include the (enc|dec)/uint(32|64).c files, see BurningEnlightenment/base64-cmake@d405e70

@aklomp
Copy link
Owner

aklomp commented Jul 5, 2016

Good catch, I totally overlooked that. I just rebased that commit and the "MSVC OpenMP incompetence" (seriously, MSVC, what the hell) onto master and pushed it. Congratulations on your first in-tree commits ;)

I decided to rebase instead of merge, because your branch is built on top of the Cmake layer, and I'm not quite ready to tackle that just yet.

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.

3 participants