Windows compatibility fixes (spaces, stubs, includes) #86
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| #pragma once | ||
| #ifdef _WIN32 | ||
|
|
||
| #include <io.h> | ||
| #include <fcntl.h> | ||
| #include <stdlib.h> | ||
| #include <errno.h> | ||
| #include <cstring> | ||
|
|
||
| #define strncasecmp _strnicmp | ||
|
|
||
| #ifndef getdelim | ||
|
Owner
There was a problem hiding this comment. What is this for? Is it possible that getdelim is sometimes defined as a macro, sometimes not, but never defined as a function?
Author
There was a problem hiding this comment. That is so it uses the windows equivalent of strncasecmp, I'll work on all these changes and make it suitable. I have a busy week coming up and start work in 2 hours, I'l try to clean all this up and explain on Saturday. Sorry for the lack of explanation on these changes and having to wait until Saturday.
Author
There was a problem hiding this comment. I think I misunderstood your question. I added the #ifndef getdelim because I wanted to prevent a duplicate definition if the platform already supplied getdelim(). I realize now that this only checks for a preprocessor macro, not whether the function is already provided.
Owner
There was a problem hiding this comment. Exactly. If the _WIN32 macro check is not enough, we should rely on CMake because that’s precisely what it’s for. See CheckFunctionExists. |
||
| static inline ssize_t getdelim(char** lineptr, size_t* n, int delim, FILE* stream) { | ||
|
Owner
There was a problem hiding this comment. This function is huge for an inline function. You could move the definition into system.cc and add the declaration into opustags.h instead so that it is built and linked like a regular function. By the way, where does the implementation come from?
Author
There was a problem hiding this comment. I wrote the getdelim (and mkstemps) implementations myself to get the Windows build working. I looked at a few public versions for reference while writing it, but this is my own code. Happy to move both functions out of the header into system.cc (and just declare them in the header or in opustags.h) if you prefer that style. I can also clean them up further if you’d like.
Owner
There was a problem hiding this comment. Thank you. Since C++ is loaded with footguns so I’ll make sure to review it attentively, unless we find a battle-tested alternative. Maybe we could replace getdelim with std::getline, provided the code does not rely too much on stdio. Regarding mkstemps, Windows seems to have _mktemp that could do some of the work.
Author
There was a problem hiding this comment. Nice, I will look into _mktemp, Windows seems very messy. I'll look into all this more thoroughly on Saturday including using getline instead of getdelim. My PR was pretty much out of excitement that I actually got it to compile and work on Windows. I've been wanting / needing opustags for Windows for a while. |
||
| if (lineptr == nullptr || n == nullptr || stream == nullptr) { | ||
| errno = EINVAL; | ||
| return -1; | ||
| } | ||
| size_t capacity = (*n > 0) ? *n : 128; | ||
| if (*lineptr == nullptr) { | ||
| *lineptr = static_cast<char*>(malloc(capacity)); | ||
| if (*lineptr == nullptr) return -1; | ||
| *n = capacity; | ||
| } | ||
| size_t pos = 0; | ||
| int c; | ||
| while ((c = fgetc(stream)) != EOF) { | ||
| if (pos + 1 >= capacity) { | ||
| capacity *= 2; | ||
| char* newbuf = static_cast<char*>(realloc(*lineptr, capacity)); | ||
| if (newbuf == nullptr) return -1; | ||
| *lineptr = newbuf; | ||
| *n = capacity; | ||
| } | ||
| (*lineptr)[pos++] = static_cast<char>(c); | ||
| if (c == delim) break; | ||
| } | ||
| if (pos == 0 && c == EOF) return -1; | ||
| (*lineptr)[pos] = '\0'; | ||
| return static_cast<ssize_t>(pos); | ||
| } | ||
| #endif | ||
|
|
||
| #ifndef mkstemps | ||
| static inline int mkstemps(char* tmpl, int suffixlen) { | ||
| size_t len = strlen(tmpl); | ||
| if (len < (size_t)(6 + suffixlen)) return -1; | ||
| char* xxxx = tmpl + len - 6 - suffixlen; | ||
| static const char chars[] = "abcdefghijklmnopqrstuvwxyz0123456789"; | ||
| for (int attempt = 0; attempt < 100; ++attempt) { | ||
| for (int i = 0; i < 6; ++i) | ||
| xxxx[i] = chars[rand() % (sizeof(chars) - 1)]; | ||
| int fd = _open(tmpl, _O_CREAT | _O_EXCL | _O_RDWR | _O_BINARY, _S_IREAD | _S_IWRITE); | ||
| if (fd != -1) return fd; | ||
| if (errno != EEXIST) return -1; | ||
| } | ||
| return -1; | ||
| } | ||
| #endif | ||
|
|
||
| #endif | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why check both
_WIN32and__MINGW32__. Isn’t _WIN32 enough?