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

There's Windows code in the Unix code #115

Open
guijan opened this issue Jun 26, 2024 · 1 comment
Open

There's Windows code in the Unix code #115

guijan opened this issue Jun 26, 2024 · 1 comment

Comments

@guijan
Copy link
Contributor

guijan commented Jun 26, 2024

In this large block of code that compromises a single #if defined(_WIN32) #else #endif conditional:

#if defined(_WIN32)
std::unique_ptr<ByteStream> ByteStream_OpenFileStream(const char* fileName, u32 openMode)
{
if ((openMode & (BYTESTREAM_OPEN_CREATE | BYTESTREAM_OPEN_WRITE)) == BYTESTREAM_OPEN_WRITE)
{
// if opening with write but not create, the path must exist.
if (!path_is_valid(fileName))
return nullptr;
}
char modeString[16];
u32 modeStringLength = 0;
if (openMode & BYTESTREAM_OPEN_WRITE)
{
// if the file exists, use r+, otherwise w+
// HACK: if we're not truncating, and the file exists (we want to only update it), we still have to use r+
if (!path_is_valid(fileName))
{
modeString[modeStringLength++] = 'w';
if (openMode & BYTESTREAM_OPEN_READ)
modeString[modeStringLength++] = '+';
}
else
{
modeString[modeStringLength++] = 'r';
modeString[modeStringLength++] = '+';
}
modeString[modeStringLength++] = 'b';
}
else if (openMode & BYTESTREAM_OPEN_READ)
{
modeString[modeStringLength++] = 'r';
modeString[modeStringLength++] = 'b';
}
// doesn't work with _fdopen
if (!(openMode & BYTESTREAM_OPEN_ATOMIC_UPDATE))
{
if (openMode & BYTESTREAM_OPEN_STREAMED)
modeString[modeStringLength++] = 'S';
else if (openMode & BYTESTREAM_OPEN_SEEKABLE)
modeString[modeStringLength++] = 'R';
}
modeString[modeStringLength] = 0;
if (openMode & BYTESTREAM_OPEN_CREATE_PATH)
{
u32 i;
u32 fileNameLength = static_cast<u32>(std::strlen(fileName));
char* tempStr = (char*)alloca(fileNameLength + 1);
// check if it starts with a drive letter. if so, skip ahead
if (fileNameLength >= 2 && fileName[1] == ':')
{
if (fileNameLength <= 3)
{
// create a file called driveletter: or driveletter:\ ? you must be crazy
i = fileNameLength;
}
else
{
std::memcpy(tempStr, fileName, 3);
i = 3;
}
}
else
{
// start at beginning
i = 0;
}
// step through each path component, create folders as necessary
for (; i < fileNameLength; i++)
{
if (i > 0 && (fileName[i] == '\\' || fileName[i] == '/'))
{
// terminate the string
tempStr[i] = '\0';
// check if it exists
if (!path_is_valid(tempStr))
{
if (errno == ENOENT)
{
// try creating it
if (!path_mkdir(tempStr)) // no point trying any further down the chain
break;
}
else // if (errno == ENOTDIR)
{
// well.. someone's trying to open a fucking weird path that is comprised of both directories and files...
// I aint sticking around here to find out what disaster awaits... let fopen deal with it
break;
}
}
// append platform path seperator
#if defined(_WIN32)
tempStr[i] = '\\';
#else
tempStr[i] = '/';
#endif
}
else
{
// append character to temp string
tempStr[i] = fileName[i];
}
}
}
if (openMode & BYTESTREAM_OPEN_ATOMIC_UPDATE)
{
// generate the temporary file name
u32 fileNameLength = static_cast<u32>(std::strlen(fileName));
char* temporaryFileName = (char*)alloca(fileNameLength + 8);
std::snprintf(temporaryFileName, fileNameLength + 8, "%s.XXXXXX", fileName);
// fill in random characters
_mktemp_s(temporaryFileName, fileNameLength + 8);
wchar_t *wideTemporaryFileName = utf8_to_utf16_string_alloc(temporaryFileName);
// massive hack here
DWORD desiredAccess = GENERIC_WRITE;
if (openMode & BYTESTREAM_OPEN_READ)
desiredAccess |= GENERIC_READ;
HANDLE hFile =
CreateFileW(wideTemporaryFileName, desiredAccess, FILE_SHARE_DELETE, NULL, CREATE_NEW, 0, NULL);
if (hFile == INVALID_HANDLE_VALUE)
{
free(wideTemporaryFileName);
return nullptr;
}
// get fd from this
int fd = _open_osfhandle(reinterpret_cast<intptr_t>(hFile), 0);
if (fd < 0)
{
CloseHandle(hFile);
DeleteFileW(wideTemporaryFileName);
free(wideTemporaryFileName);
return nullptr;
}
// convert to a stream
FILE* pTemporaryFile = _fdopen(fd, modeString);
if (!pTemporaryFile)
{
_close(fd);
DeleteFileW(wideTemporaryFileName);
free(wideTemporaryFileName);
return nullptr;
}
// create the stream pointer
std::unique_ptr<AtomicUpdatedFileByteStream> pStream =
std::make_unique<AtomicUpdatedFileByteStream>(pTemporaryFile, fileName, temporaryFileName);
// do we need to copy the existing file into this one?
if (!(openMode & BYTESTREAM_OPEN_TRUNCATE))
{
RFILE* pOriginalFile = FileSystem::OpenRFile(fileName, "rb");
if (!pOriginalFile)
{
// this will delete the temporary file
pStream->Discard();
free(wideTemporaryFileName);
return nullptr;
}
static const size_t BUFFERSIZE = 4096;
u8 buffer[BUFFERSIZE];
while (!rfeof(pOriginalFile))
{
size_t nBytes = rfread(buffer, BUFFERSIZE, sizeof(u8), pOriginalFile);
if (nBytes == 0)
break;
if (pStream->Write(buffer, (u32)nBytes) != (u32)nBytes)
{
pStream->Discard();
rfclose(pOriginalFile);
free(wideTemporaryFileName);
return nullptr;
}
}
// close original file
rfclose(pOriginalFile);
}
free(wideTemporaryFileName);
// return pointer
return pStream;
}
else
{
// forward through
FILE* pFile = FileSystem::OpenCFile(fileName, modeString);
if (!pFile)
return nullptr;
return std::make_unique<FileByteStream>(pFile);
}
}
#else
std::unique_ptr<ByteStream> ByteStream_OpenFileStream(const char* fileName, u32 openMode)
{
if ((openMode & (BYTESTREAM_OPEN_CREATE | BYTESTREAM_OPEN_WRITE)) == BYTESTREAM_OPEN_WRITE)
{
// if opening with write but not create, the path must exist.
if (!path_is_valid(fileName))
return nullptr;
}
char modeString[16];
u32 modeStringLength = 0;
if (openMode & BYTESTREAM_OPEN_WRITE)
{
if (openMode & BYTESTREAM_OPEN_TRUNCATE)
modeString[modeStringLength++] = 'w';
else
modeString[modeStringLength++] = 'a';
modeString[modeStringLength++] = 'b';
if (openMode & BYTESTREAM_OPEN_READ)
modeString[modeStringLength++] = '+';
}
else if (openMode & BYTESTREAM_OPEN_READ)
{
modeString[modeStringLength++] = 'r';
modeString[modeStringLength++] = 'b';
}
modeString[modeStringLength] = 0;
if (openMode & BYTESTREAM_OPEN_CREATE_PATH)
{
u32 i;
const u32 fileNameLength = static_cast<u32>(std::strlen(fileName));
char* tempStr = (char*)alloca(fileNameLength + 1);
#if defined(_WIN32)
// check if it starts with a drive letter. if so, skip ahead
if (fileNameLength >= 2 && fileName[1] == ':')
{
if (fileNameLength <= 3)
{
// create a file called driveletter: or driveletter:\ ? you must be crazy
i = fileNameLength;
}
else
{
std::memcpy(tempStr, fileName, 3);
i = 3;
}
}
else
{
// start at beginning
i = 0;
}
#endif
// step through each path component, create folders as necessary
for (i = 0; i < fileNameLength; i++)
{
if (i > 0 && (fileName[i] == '\\' || fileName[i] == '/') && fileName[i] != ':')
{
// terminate the string
tempStr[i] = '\0';
// check if it exists
if (!path_is_valid(tempStr))
{
if (errno == ENOENT)
{
// try creating it
if (!path_mkdir(tempStr)) // no point trying any further down the chain
break;
}
else // if (errno == ENOTDIR)
{
// well.. someone's trying to open a fucking weird path that is comprised of both directories and
// files... I aint sticking around here to find out what disaster awaits... let fopen deal with it
break;
}
}
// append platform path seperator
#if defined(_WIN32)
tempStr[i] = '\\';
#else
tempStr[i] = '/';
#endif
}
else
{
// append character to temp string
tempStr[i] = fileName[i];
}
}
}
if (openMode & BYTESTREAM_OPEN_ATOMIC_UPDATE)
{
// generate the temporary file name
const u32 fileNameLength = static_cast<u32>(std::strlen(fileName));
char* temporaryFileName = (char*)alloca(fileNameLength + 8);
std::snprintf(temporaryFileName, fileNameLength + 8, "%s.XXXXXX", fileName);
// fill in random characters
#if defined(__linux__) || defined(__ANDROID__) || defined(__APPLE__)
mkstemp(temporaryFileName);
#else
mktemp(temporaryFileName);
#endif
// open the file
std::FILE* pTemporaryFile = std::fopen(temporaryFileName, modeString);
if (pTemporaryFile == nullptr)
return nullptr;
// create the stream pointer
std::unique_ptr<AtomicUpdatedFileByteStream> pStream =
std::make_unique<AtomicUpdatedFileByteStream>(pTemporaryFile, fileName, temporaryFileName);
// do we need to copy the existing file into this one?
if (!(openMode & BYTESTREAM_OPEN_TRUNCATE))
{
RFILE* pOriginalFile = rfopen(fileName, "rb");
if (!pOriginalFile)
{
// this will delete the temporary file
pStream->SetErrorState();
return nullptr;
}
static const size_t BUFFERSIZE = 4096;
u8 buffer[BUFFERSIZE];
while (!rfeof(pOriginalFile))
{
size_t nBytes = rfread(buffer, BUFFERSIZE, sizeof(u8), pOriginalFile);
if (nBytes == 0)
break;
if (pStream->Write(buffer, (u32)nBytes) != (u32)nBytes)
{
pStream->SetErrorState();
rfclose(pOriginalFile);
return nullptr;
}
}
// close original file
rfclose(pOriginalFile);
}
// return pointer
return pStream;
}
else
{
std::FILE* pFile = std::fopen(fileName, modeString);
if (!pFile)
return nullptr;
return std::make_unique<FileByteStream>(pFile);
}
}
#endif

In the #else branch:
#else
std::unique_ptr<ByteStream> ByteStream_OpenFileStream(const char* fileName, u32 openMode)
{
if ((openMode & (BYTESTREAM_OPEN_CREATE | BYTESTREAM_OPEN_WRITE)) == BYTESTREAM_OPEN_WRITE)
{
// if opening with write but not create, the path must exist.
if (!path_is_valid(fileName))
return nullptr;
}
char modeString[16];
u32 modeStringLength = 0;
if (openMode & BYTESTREAM_OPEN_WRITE)
{
if (openMode & BYTESTREAM_OPEN_TRUNCATE)
modeString[modeStringLength++] = 'w';
else
modeString[modeStringLength++] = 'a';
modeString[modeStringLength++] = 'b';
if (openMode & BYTESTREAM_OPEN_READ)
modeString[modeStringLength++] = '+';
}
else if (openMode & BYTESTREAM_OPEN_READ)
{
modeString[modeStringLength++] = 'r';
modeString[modeStringLength++] = 'b';
}
modeString[modeStringLength] = 0;
if (openMode & BYTESTREAM_OPEN_CREATE_PATH)
{
u32 i;
const u32 fileNameLength = static_cast<u32>(std::strlen(fileName));
char* tempStr = (char*)alloca(fileNameLength + 1);
#if defined(_WIN32)
// check if it starts with a drive letter. if so, skip ahead
if (fileNameLength >= 2 && fileName[1] == ':')
{
if (fileNameLength <= 3)
{
// create a file called driveletter: or driveletter:\ ? you must be crazy
i = fileNameLength;
}
else
{
std::memcpy(tempStr, fileName, 3);
i = 3;
}
}
else
{
// start at beginning
i = 0;
}
#endif
// step through each path component, create folders as necessary
for (i = 0; i < fileNameLength; i++)
{
if (i > 0 && (fileName[i] == '\\' || fileName[i] == '/') && fileName[i] != ':')
{
// terminate the string
tempStr[i] = '\0';
// check if it exists
if (!path_is_valid(tempStr))
{
if (errno == ENOENT)
{
// try creating it
if (!path_mkdir(tempStr)) // no point trying any further down the chain
break;
}
else // if (errno == ENOTDIR)
{
// well.. someone's trying to open a fucking weird path that is comprised of both directories and
// files... I aint sticking around here to find out what disaster awaits... let fopen deal with it
break;
}
}
// append platform path seperator
#if defined(_WIN32)
tempStr[i] = '\\';
#else
tempStr[i] = '/';
#endif
}
else
{
// append character to temp string
tempStr[i] = fileName[i];
}
}
}
if (openMode & BYTESTREAM_OPEN_ATOMIC_UPDATE)
{
// generate the temporary file name
const u32 fileNameLength = static_cast<u32>(std::strlen(fileName));
char* temporaryFileName = (char*)alloca(fileNameLength + 8);
std::snprintf(temporaryFileName, fileNameLength + 8, "%s.XXXXXX", fileName);
// fill in random characters
#if defined(__linux__) || defined(__ANDROID__) || defined(__APPLE__)
mkstemp(temporaryFileName);
#else
mktemp(temporaryFileName);
#endif
// open the file
std::FILE* pTemporaryFile = std::fopen(temporaryFileName, modeString);
if (pTemporaryFile == nullptr)
return nullptr;
// create the stream pointer
std::unique_ptr<AtomicUpdatedFileByteStream> pStream =
std::make_unique<AtomicUpdatedFileByteStream>(pTemporaryFile, fileName, temporaryFileName);
// do we need to copy the existing file into this one?
if (!(openMode & BYTESTREAM_OPEN_TRUNCATE))
{
RFILE* pOriginalFile = rfopen(fileName, "rb");
if (!pOriginalFile)
{
// this will delete the temporary file
pStream->SetErrorState();
return nullptr;
}
static const size_t BUFFERSIZE = 4096;
u8 buffer[BUFFERSIZE];
while (!rfeof(pOriginalFile))
{
size_t nBytes = rfread(buffer, BUFFERSIZE, sizeof(u8), pOriginalFile);
if (nBytes == 0)
break;
if (pStream->Write(buffer, (u32)nBytes) != (u32)nBytes)
{
pStream->SetErrorState();
rfclose(pOriginalFile);
return nullptr;
}
}
// close original file
rfclose(pOriginalFile);
}
// return pointer
return pStream;
}
else
{
std::FILE* pFile = std::fopen(fileName, modeString);
if (!pFile)
return nullptr;
return std::make_unique<FileByteStream>(pFile);
}
}
#endif

You can see that there are #if defined(_WIN32) checks which will never be true:
#if defined(_WIN32)
// check if it starts with a drive letter. if so, skip ahead
if (fileNameLength >= 2 && fileName[1] == ':')
{
if (fileNameLength <= 3)
{
// create a file called driveletter: or driveletter:\ ? you must be crazy
i = fileNameLength;
}
else
{
std::memcpy(tempStr, fileName, 3);
i = 3;
}
}
else
{
// start at beginning
i = 0;
}
#endif

#if defined(_WIN32)
tempStr[i] = '\\';
#else

We have the same situation in the Windows code, with conditional compilation to handle the Unix path separator which will never trigger. It looks like these branches were copypasted from the same code.

The low effort fix would be to simply remove these unused branches, however, the real fix would be to unify the Windows and Unix code because it's almost the same, it appears the Windows code only differs in having some character set conversion to deal with Windows versions without UTF-8 support, the rest of the code shouldn't be inside a preprocessor conditional.

@DarthMew
Copy link
Collaborator

Removed the redundant code via: 092b812

If you want to simplify the code further, be my guest.

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

No branches or pull requests

2 participants