Get current working dir fix #431
Conversation
Logic error was in GetCurrentWorkingDirectory. Also removed a global variable that was used to cache this value and the initialization of the global variable. There's no need to cache this value as the call is made very infrequently. Also fixed up some formatting.
It turns out that the reason the static initialization code was written is to ensure that the thread-unsafe code for finding the current working directory (specifically on windows) happens on a single thread during static initialization.
util/src/FileSystem.cpp
Outdated
|
|
||
| std::string GetCurrentWorkingDirectory() | ||
| { | ||
| return s_CurrWorkingDir; | ||
| static const std::string currentWorkingDir = InitCurrentWorkingDirectory(); |
There was a problem hiding this comment.
Using a local static here (instead of a static global as before) makes the code potentially racy with other code executing in a different thread, trying to change the current working directory. The original usage of a static global variable avoids this issue. (Local statics are initialized on first use, which may well be after main() was called, which means their initialization code is potentially executed while other threads are already created, possibly calling OS functions like SetCurrentDirectory. Global statics do not have this problem).
There was a problem hiding this comment.
I don't think that's entirely correct. Here's what the standard says:
"It is implementation-defined whether or not the dynamic initialization (8.5, 9.4, 12.1, 12.6.1) of an object of namespace scope is done before the first statement of main. If the initialization is deferred to some point in time after the first statement of main, it shall occur before the first use of any function or object defined in the same translation unit as the object to be initialized"
In other words, I don't think the standard guarantees that static global initialization happens before main. And even if it did, the order of initialization is not guaranteed, and customer written constructors for global object could also call SetCurrentDirectory, and this may or may not happen before the initialization of s_CurrentWorkingDirectory in FileSystem.cpp.
I will change it back if you think it's better.
There was a problem hiding this comment.
I had a MSDN specific article in mind, but I cannot find a reference anymore (maybe my memory doesn't serve me well). Thanks for digging out the standard paragraph. I agree that this doesn't guarantee what we would actually need.
After all, it doesn't look like we can reliably retrieve the current working directory on Windows. My original point about main() is moot, since the framework is typically loaded (and initialized) as a shared library from some user's program, which can occur well after main was called.
Long story short: Your current solution is not worse than before, but both are flawed. I think we should (in a subsequent task) remove that code completely and either make the framework operate without knowing about the current working directory, or require that information to be passed in during framework initialization.
modified GetCurrentWorkingDir to not cause an infinite loop if an error occurs other than ERANGE.