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

Allow compiling the gem with MSVC (which is not C99 compliant) #43

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jmarrec
Copy link

@jmarrec jmarrec commented Mar 25, 2020

I realize this is probably a corner case (using MSVC to compile a ruby gem), but that's he situation I am in: I have to use MSVC to compile this gem as we have a C++ application that has to be built on MSVC and one thing it does is to create a CLI based on ruby and embedded some gems.

The fun stuff is that MSVC (including the 2019 16.5.1 that I use which is fully up to date) isn't C99 compliant, it's C89 compliant. So in order to be able to build the C extension in this repo, I had to do the following changes:

  • Do not use Variable-length arrays (VLA) but instead use a heap array with malloc & free
  • Redefine a macro to pass the type

Edit: Well actually, what a coincidence, I see this PR actually fixes an open issue. Fix #41

* Do not use Variable-length arrays (VLA) but instead use a heap array with malloc & free
* Redefine a macro
@jmarrec
Copy link
Author

jmarrec commented Mar 25, 2020

Benchmarked on linux after changes to see impact of these changes:

develop: 0.097458
First pass: 0.144719
second pass: 0.099472

jaro_winkler (9e1d7a0) 0.097458 0.000000 0.097458 ( 0.097460)
jaro_winkler (a742b86) 0.147391 0.000000 0.147391 ( 0.147397)
jaro_winkler (22a60aa) 0.099472 0.000000 0.099472 ( 0.099475)

Had to remove 22a60aa (which was a commit where I tried to remove the memset call) as tests were failing.

@jmarrec
Copy link
Author

jmarrec commented Mar 26, 2020

Hum I guess an alternative is to declare the arrays as fixed size, large enough.

char short_codes_flag[1000];
char long_codes_flag[1000];

@jmarrec
Copy link
Author

jmarrec commented Mar 26, 2020

alloca seems to work, but haven't tested on MSVC

jaro_winkler (12dc642) 0.108995 0.000000 0.108995 ( 0.108998)

@jmarrec
Copy link
Author

jmarrec commented Mar 26, 2020

Ok, used alloca instead, back to normal in terms of performance:

before: jaro_winkler (9e1d7a0) 0.097458 0.000000 0.097458 ( 0.097460)
after : jaro_winkler (f1ca425) 0.097654 0.000000 0.097654 ( 0.097674)

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.

Windows mswin build - failing
1 participant