Windows build fails: undeclared identifier 'close' about cld2

So it wasn't quite as trivial as hoped, because mmap wants a file descriptor (int) instead
of a FILE*. Using fopen() and fclose() is the portable way to do things, but under
the covers they do a bunch of buffering and such that we don't really need for our
mmap call. On the bright side it seems like using fileno() from stdio.h allows us to
get the file descriptor, and I can't imagine that we will have any problems with mmap'ing
the region read-only so long as we are also fopen()'ing in "r" (read-only) mode, even
if it mmaps the same region under the covers. So hopefully all is well.

All unit tests pass in both dynamic and non-dynamic modes with the attached patch,
so I think everything should be alright.

Reported by [email protected] on 2014-08-13 10:02:42

---

- _Attachment: [patch](https://storage.googleapis.com/google-code-attachments/cld2/issue-19/comment-1/patch)_

I had a pro/con discussion with another engineer on this, and here is the summary: the
patch will fix the problem, but is a little sketchy because of the mixing of fopen()
with mmap(). It would be better to #include unistd.h, and continue using open() instead
of fopen(); but this is specific to Unix and wouldn't work on Windows. That said, the
code is *already* broken on Windows because of the use of sys/mman.h (for mmap).

So: If we use unistd.h, we deepen the problems for Windows. If we use fopen(), we do
something questionable for non-Windows (it works, but it isn't really the right thing
to do).

I think the answer here is to use unistd.h for linux and use a workaround like this
to fix win32:

// Based on definitions from http://sourceforge.net/p/predef/wiki/OperatingSystems/:
#ifdef _WIN32
#include <io.h>
#define OPEN _open
#define CLOSE _close
#else
#include "unistd.h"
#define OPEN open
#define CLOSE close
#endif

We can extend this solution to fix the mmap problem as well, in the near future.

Reported by [email protected] on 2014-08-13 11:12:51

I've created issue 20 to track the overall Windows compatibility problem for the dynamic
data loader.

Reported by [email protected] on 2014-08-13 11:40:31

Here is a new patch based on #2 above. As described in issue 20, it disables support
for win32 in the "file"-based dynamic data apis; only raw pointer mode will work on
win32 until we have proper mmap support there.

So to be clear, we didn't end up changing the open() to fopen(), because open() makes
sense for this use case. Updating the bug description accordingly.

Reported by [email protected] on 2014-08-13 12:31:26

---

- _Attachment: [patch2](https://storage.googleapis.com/google-code-attachments/cld2/issue-19/comment-4/patch2)_

Should be fixed in r166.

Reported by [email protected] on 2014-08-13 12:35:43

• Status changed: Fixed

Issue 23 has been merged into this issue.

Reported by [email protected] on 2014-08-29 11:33:08