Proposal: Refactoring platform-specific code in util.h/util.cpp #12697
Description
There is a fair amount of code in util.cpp that by its nature has to be done completely differently on Windows and Unix (and sometimes differently on Linux, BSD, etc). There's actually more of this kind of stuff that I want to add e.g. see some of my other outstanding PRs like #12618 and #12696.
Most of the functions like this are written with a top-level #ifdef that dispatches based on Windows or non-Windows:
int RaiseFileDescriptorLimit(int nMinFD) {
#if defined(WIN32)
return 2048;
#else
struct rlimit limitFD;
if (getrlimit(RLIMIT_NOFILE, &limitFD) != -1) {
if (limitFD.rlim_cur < (rlim_t)nMinFD) {
limitFD.rlim_cur = nMinFD;
if (limitFD.rlim_cur > limitFD.rlim_max)
limitFD.rlim_cur = limitFD.rlim_max;
setrlimit(RLIMIT_NOFILE, &limitFD);
getrlimit(RLIMIT_NOFILE, &limitFD);
}
return limitFD.rlim_cur;
}
return nMinFD; // getrlimit failed, assume it's fine
#endif
In a few cases this kind of logic will be interspersed in functions that are otherwise fairly platform independent, e.g. ArgsManager::ParseParameters has logic to convert Unix-style filenames to Windows style filenames:
void ArgsManager::ParseParameters(int argc, const char* const argv[])
{
// ...
#ifdef WIN32
boost::to_lower(str);
if (boost::algorithm::starts_with(str, "/"))
str = "-" + str.substr(1);
#endif
// ...
}There are also a few where we need to do different things if we're on Linux, macOS, etc.
Proposal
My proposal is to take all of the functions that are specific to Windows or Unix, and create a common set of methods in a new header file called util_platform.h. This would contain prototypes for methods like RenameOver(), FileCommit(), etc.
Then we'd have two implementation files, util_windows.cpp and util_posix.cpp that implement these methods. The building/linking of util_windows.cpp or util_posix.cpp would be handled using AM_CONDITIONAL. My feeling is that the POSIX stuff is similar enough that we can avoid a util_mac.cpp, util_linux.cpp, etc. for now (and hopefully forever).
When you add a new function prototype to util_platform.h you need to write implementations in both util_windows.cpp and util_posix.cpp. In some cases that will mean there are empty functions defined which is fine.
I think if we do this correctly there should be ZERO statements that are checking for #ifdef WIN32 in the rest of the code. As an example, previously I showed some code from ArgsManager that was munging Unix filenames to Windows filenames. I think that should be handled in a function e.g. named MungeUnixFilename() and then on Windows it would implemented with the existing lowercase/replace slash logic, and on POSIX systems it would be a function that just returned the original string unchanged.