Extend pathtools cross-platform #337
Conversation
src/configuration.cpp
Outdated
| @@ -193,12 +193,14 @@ ssr::conf_struct ssr::configuration(int& argc, char* argv[]) | |||
| // load system-wide config file (Linux et al.) | |||
| load_config_file("/etc/ssr.conf",conf); | |||
| // load user config file (Mac) | |||
| std::string filename = getenv("HOME"); | |||
| std::string filename = pathtools::get_home_dir().string(); | |||
There was a problem hiding this comment.
Wouldn't it make more sense to keep using a path here?
Then the following line could use / to append components.
There was a problem hiding this comment.
Unfortunately the cross-compiler doesn't like these implicit path to string conversions...
There was a problem hiding this comment.
Sorry, I wasn't clear.
I'm not talking about any implicit conversions (those are bad!), I'm talking about never using std::string at all.
get_home_dir() would return a path, which would then be manipulated as such, and in the very end, .c_str() would be called on it.
There was a problem hiding this comment.
That works, too. It seems that we still need an explicit string() conversion before c_str(), as path::c_str() is equivalent to native().c_str().
https://en.cppreference.com/w/cpp/filesystem/path/native
The native type on windows is w_string, and its pointer was not compatible to const char*.
There was a problem hiding this comment.
Oh, that's unfortunate. It's interesting how C++ reliably manages to have the least expected behavior by default ... but I guess string().c_str() is fine.
src/configuration.cpp
Outdated
| filename += "/Library/SoundScapeRenderer/ssr.conf"; | ||
| filename = pathtools::normalize_path(filename); | ||
| load_config_file(filename.c_str(),conf); |
There was a problem hiding this comment.
filename could still be a path here, which also provides a .c_str() method.
src/pathtools.h
Outdated
| **/ | ||
| inline fs::path get_home_dir() | ||
| { | ||
| fs::path p_home = fs::current_path().root_path(); |
There was a problem hiding this comment.
I'm not sure about this fallback case ... why would we return root_path()?
This seems wrong to me.
I think we could take inspiration from root_path() and return path() as a fallback.
This way, the caller could check if there is a HOME directory or not.
We would of course also have to check this when we are using get_home_dir().
I guess for now we were assuming that $HOME is always available, which might be true in most cases, but we might as well add a check for this case while we are at it.
There was a problem hiding this comment.
root_path is indeed not the best solution. How about current_path()? That's also easy to test and perhaps a better fallback. Say a file can also be located in the installation folder, and could then be found as a fallback
There was a problem hiding this comment.
$HOME is actually not available on Windows!
There was a problem hiding this comment.
That's also easy to test
I disagree. It seems dangerous, because the current path might change at any point.
and perhaps a better fallback.
I don't think the current path should be involved, it has nothing to do with the HOME directory.
If no HOME directory is defined, this function should basically return an error. If path() is returned, this would mean it didn't work and there is no HOME dir.
There was a problem hiding this comment.
Ok, I see! I updated to return path() now, in case where it fails
|
@chris-hld What do you think about my latest comments? To avoid confusion about what I meant, I have implemented them in ed03a85 and e31b1e5. |
Introduce
get_home_dir()andnormalize_path().