-
Notifications
You must be signed in to change notification settings - Fork 503
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
Rely on std::filesystem for file_utils #3042
base: develop
Are you sure you want to change the base?
Rely on std::filesystem for file_utils #3042
Conversation
First attempt was to remove `file_utils` in favor of direct calls to things like `std::filesystem::exists` and ``std::filesystem::path::get_extension`. However, many places in openmc use strings to store paths to files and directories which would require patterns that previously look like ```cpp if (!file_exists(member_string_var_)) { ... } ``` where a class stores a string file path at `member_string_var_` to change to ```cpp std::filesystem::path p(member_string_var_); if (!std::filesystem::exists(p)) { ... } ``` This second pattern would be repeated in a lot of the code base, so keeping `file_exists(std::string)` means less repeated code. An alternative is to use `std::filesystem::path` objects to represent paths to files and directories. This would work pretty easily in at least two locations: - `Library::path_` - `DAGUniverse::filename_` But the primary source of "strings for paths" is due to various settings, e.g., `settings::path_input`. I don't believe those can be converted to `std::filesystem::path` objects because they must be more portable to C and, by extension, Python. Closes openmc-dev#3041
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.
Nice. This revised code is objectively cleaner. Really, shouldn't extern std::string
not even work for the C API? If it's working, it's by coincidence. Casting an std::string
to char*
and using it through a C API may or may not give the right behavior.
Maybe it'd be worth considering changing the global variables to be paths, then altering the C API to work with that, rather than directly manipulating char* variables in the C API. Whoever else reviews this can decide that matter!
Sorry about the weird behavior with the numeric "extensions" not counting as extensions.
Always writes an h5 formatted file to the provided filepath, even if the extension is not .h5
Always writes an h5 formatted file to the provided filepath, even if the extension is not .h5
Thanks @gridley! Regarding
I'm open to the idea as we do a lot of path manipulation off those settings. Things like
But, that's also a larger API change that could have more knock on effects that I'm not considering |
Previous implementation returned the substring up to and including the final directory separator. This is not consistent with `std::filesystem::path::parent_path`
This reverts commit b6b9765.
Avoids needing to modify a user-provided file name in write_source_point and write_mcnp_source_point
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.
@drewejohnson This looks good to me, but there is a conflict from the recent merging of #3055. Can you merge develop into your branch and resolve the conflict?
Description
file_utils
is more like a thin wrapper overstd::filesystem
primarily because we have a lot of strings describing files being passed around.First attempt was to remove
file_utils
in favor of direct calls to things likestd::filesystem::exists
andstd::filesystem::path::get_extension
. However, many places in openmc use strings to store paths to files and directories which would require patterns that previously look likeif (!file_exists(member_string_var_)) { ... }
where a class stores a string file path at
member_string_var_
. These would becomeThis second pattern would be repeated in a lot of the code base, so keeping
file_exists(const std::string&)
means less repeated code.An alternative is to use
std::filesystem::path
objects to represent paths to files and directories. This would work pretty easily in at least two locations:Library::path_
DAGUniverse::filename_
But the primary source of "strings for paths" is due to various settings, e.g.,
settings::path_input
. I don't believe those can be converted tostd::filesystem::path
objects because they must be more portable to C and, by extension, Python.Fixes #3041
Checklist
file_utils
.