Fix filesystem helpers for directory manipulation. #30
Conversation
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
CI up to
|
| inline bool remove(const path & p) | ||
| { | ||
| #ifdef _WIN32 | ||
| return _rmdir(p.string().c_str()) == 0; |
There was a problem hiding this comment.
What if the path is a file?
remove is supposed to delete both files and empty directories.
See https://en.cppreference.com/w/cpp/filesystem/remove.
There was a problem hiding this comment.
That's why I said it's a partial implementation. We can make it full, I simply didn't have a use case.
There was a problem hiding this comment.
Add a comment in the docsrting of the function. If not, later users might not realize.
| @@ -183,16 +183,31 @@ inline bool create_directories(const path & p) | |||
| { | |||
| path p_built; | |||
|
|
|||
There was a problem hiding this comment.
nit: delete this new line and add one between status and the for loop?
There was a problem hiding this comment.
A bit of a taste nit, but sure, either way is fine. See 6ac7378.
| #else | ||
| mkdir(p_built.string().c_str(), S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH); | ||
| status = mkdir(p_built.string().c_str(), S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH); |
There was a problem hiding this comment.
According to create_directory docs, the directory is created with 0777 perms (see here).
There was a problem hiding this comment.
Also, shouldn't break the loop if status is not zero?
There was a problem hiding this comment.
According to
create_directorydocs, the directory is created with 0777 perms (see here).
True. I do not know why the original author made it 0775 instead though.
Also, shouldn't break the loop if status is not zero?
It is.
There was a problem hiding this comment.
LGTM
Note that rcutils now provide rcutils_mkdir and other filesystem related functions. In the future, this class should probably just become a wrapper around the C code.
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
So, @ivanpauno has two good points:
@thomas-moulard any comments in favor or against? |
|
+1 on both raised points in order to match the behavior of |
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
|
@ivanpauno @jacobperron alright, done in 449e1a4. |
|
Re-running CI for
|
Precisely what the title says. Also added a partial
std::filesystem::remove()equivalent forrcpputils::fs::create_directories()testing purposes.