Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

use return code, and only conditionally define ftruncate #419

Merged
merged 2 commits into from
Sep 23, 2019

Conversation

eddelbuettel
Copy link
Contributor

Follow-up to yesterday's #418 while preparing CRAN upload:

  1. Need to actually use return code from ftruncate. Doh. So added minimal output, conditional on _verbose being set as well.

  2. Turns out the Windows version of g++ does not like the new definition of ftruncate as it has its own. So #if !defined wrapped around it.

With that I am good.

@erikbern
Copy link
Collaborator

These both seem a bit hacky. Probably would make more sense to make _allocate_size return a bool and throw an error on it. For the second point feels like there should be a more granular macro.

But good enough to merge for now, can look into it later!

@eddelbuettel
Copy link
Contributor Author

Actually we now use a differentiation for 'yes we are on Windows but there is more than just using MSVC' in one other place: line 38 in annoylib,h and I am actually quite grateful for it. What we need here may just be what I added: yes, add mman.h as we are on Windows but no, do not redefine ftruncate as MinGW brings it own (with a more POSIXy signature using off_t for size).

As for the reworking of _allocate_size(): sounds like a good idea. I just wanted something minimal to proceed.

(Also, turns out reverse dependency checks at CRAN threw a spanner on save/load comparison. I'll CC you in email in a minute. Could be anywhere between you upstream, me in the middle or that user downstream.)

@erikbern
Copy link
Collaborator

your PR is totally good for a merge for now. i just made a note of some things worth fixing later!

thanks!

@erikbern erikbern merged commit 8c690e0 into spotify:master Sep 23, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants