LogCleaner: Fix the scenario when FLAGS_log_dir has no '/' suffix #972
Conversation
|
@sergiud Could you please help to take a look? Thanks. |
sergiud
left a comment
sergiud
left a comment
There was a problem hiding this comment.
Thanks for the PR! One suggestion.
src/logging.cc
Outdated
| // A dir was specified, we should use it, and make sure to end with | ||
| // a directory delimiter. | ||
| const char* const dir_delim_end = | ||
| possible_dir_delim + sizeof(possible_dir_delim); | ||
| if (std::find(possible_dir_delim, dir_delim_end, | ||
| FLAGS_log_dir.back()) == dir_delim_end) { |
There was a problem hiding this comment.
| // A dir was specified, we should use it, and make sure to end with | |
| // a directory delimiter. | |
| const char* const dir_delim_end = | |
| possible_dir_delim + sizeof(possible_dir_delim); | |
| if (std::find(possible_dir_delim, dir_delim_end, | |
| FLAGS_log_dir.back()) == dir_delim_end) { | |
| // Ensure the specified path ends with a directory delimiter | |
| if (std::find(std::begin(possible_dir_delim), std::end(possible_dir_delim), | |
| FLAGS_log_dir.back()) == std::end(possible_dir_delim)) { |
You may need to include <iterator>.
There was a problem hiding this comment.
Thanks for your review, already addressed comment.
After testing <iterator> is not needed. My guess is that it is already included in an existing header file.
From https://en.cppreference.com/w/cpp/iterator/begin
It can be seen that std::begin and std::end are defined in many header files.
There was a problem hiding this comment.
That's not how it works. You do need <iterator> which defines std::begin and std::end for statically sized arrays (see IWYU why relying on indirection is a bad style). By 'may' I meant to say that I did not check whether <iterator> is already included in the touched translation unit.
There was a problem hiding this comment.
Just checked logging.cc. <iterator> must definitely be included since it is not present there yet.
There was a problem hiding this comment.
Thanks for your further explanation. <iterator> has been added.
|
Thanks! |
2 participants
Fixes #971