Replace create_temp_directory with the new create_temporary_directory#198
Conversation
- The newly added `create_temporary_directory(..)` uses std::filesystem::path and doesn't have platform-specific code. - Also deprecated `create_temp_directory(..)` and `temp_directory_path` Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
|
cc: @ahcorde, @clalancette - ready for review. |
clalancette
left a comment
There was a problem hiding this comment.
Overall, looks good. I've left two things inline to fix.
This will also have to be paired with PRs to rcl_logging and rosbag2 so that they use the non-deprecated API.
- Throw std::invalid_argument exception if base_path argument contains directory-separator. - Add missing `#include <stdexcept>` Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
|
@clalancette, I addressed your comments. |
ahcorde
left a comment
There was a problem hiding this comment.
Just a small nit, otherwise LGTM
src/filesystem_helper.cpp
Outdated
| auto it = | ||
| std::find(base_name.begin(), base_name.end(), std::filesystem::path::preferred_separator); | ||
| if (it != base_name.end()) { |
There was a problem hiding this comment.
I think we can make this simpler by using the string.find method:
| auto it = | |
| std::find(base_name.begin(), base_name.end(), std::filesystem::path::preferred_separator); | |
| if (it != base_name.end()) { | |
| if (base_name.find(std::filesystem::path::preferred_separator) != base_name.npos) { |
src/filesystem_helper.cpp
Outdated
| } | ||
| // mersenne twister random generator engine seeded with the std::random_device | ||
| std::mt19937 random_generator(std::random_device{}()); | ||
| std::uniform_int_distribution<> distribution(0, 999999); |
There was a problem hiding this comment.
I think this interval is too small, now that I look at it. When I ran some tests locally, I always got 0 as the first digit. And that makes sense, because 999999 == F423F hex, so there will never be something in the first digit. For 6 digits of hex, we should make this 0xFFFFFF:
| std::uniform_int_distribution<> distribution(0, 999999); | |
| std::uniform_int_distribution<> distribution(0, 0xFFFFFF); |
There was a problem hiding this comment.
@clalancette Good catch! I forgot that we convert in HEX and that in hex the range should be bigger.
Fixed in the 8b4087d commit.
- Replace std::find with string::find - Use bigger upper limit 0xFFFFFF for the uniform distribution. Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
|
Pulls: #198
|
- Use fs::path::generic_string() instead of the c_str() for rcpputils::fs::path construction. Signed-off-by: Michael Orlov <michael.orlov@apex.ai>
0ea95b0 to
fb089c8
Compare
|
Re-run Windows CI after addressing build failures
|
fujitatomoya
left a comment
There was a problem hiding this comment.
lgtm with green CI
Included ros2/rosbag2#1740 and ros2/rcl_logging#117 |
create_temporary_directory(..)uses std::filesystem::path and doesn't have platform-specific code.create_temp_directory(..)andtemp_directory_path