Skip to content
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

LogCleaner: Fix the scenario when FLAGS_log_dir has no '/' suffix #972

Merged

Conversation

lingbin
Copy link
Contributor

@lingbin lingbin commented Oct 10, 2023

Fixes #971

@lingbin
Copy link
Contributor Author

lingbin commented Oct 10, 2023

@sergiud Could you please help to take a look? Thanks.

Copy link
Collaborator

@sergiud sergiud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! One suggestion.

src/logging.cc Outdated
Comment on lines 2400 to 2405
// 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checked logging.cc. <iterator> must definitely be included since it is not present there yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your further explanation. <iterator> has been added.

@sergiud sergiud added the bug label Oct 10, 2023
@sergiud sergiud added this to the 0.7 milestone Oct 10, 2023
@sergiud
Copy link
Collaborator

sergiud commented Oct 12, 2023

Thanks!

@sergiud sergiud merged commit da97534 into google:master Oct 12, 2023
135 of 136 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LogCleaner can not delete old logs
2 participants