Skip to content
This repository was archived by the owner on Jun 30, 2025. It is now read-only.

Conversation

@sergiud
Copy link
Contributor

@sergiud sergiud commented Oct 29, 2021

As a preparation for #654.

@sergiud sergiud added this to the 0.6 milestone Oct 29, 2021
@google-cla google-cla bot added the cla: yes label Oct 29, 2021
@coveralls
Copy link
Collaborator

coveralls commented Oct 29, 2021

Coverage Status

Coverage increased (+3.5%) to 76.413% when pulling 3362cc6 on log-cleaner-tests into 9cf0eb7 on master.

@sergiud sergiud merged commit e8e40f7 into master Oct 29, 2021
@sergiud sergiud deleted the log-cleaner-tests branch October 29, 2021 21:14
dirs = GetLoggingDirectories();
} else {
size_t pos = base_filename.find_last_of(possible_dir_delim, 0,
sizeof(possible_dir_delim));
Copy link
Contributor

@aesophor aesophor Dec 6, 2021

Choose a reason for hiding this comment

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

Hello @sergiud, I think the arguments passed to find_last_of is incorrect:

  • line 1333-1334 would only search base_filename for any char in possible_dir_delim starting at index 0 for n characters, where n = sizeof(possible_dir_delim) (and in this case n = 1)
  • I think only the first argument is needed.

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. Do you have a specific test case in mind that triggers the problem? Or even better, could you submit directly a PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I have one!

I called SetLogDestination(INFO, “testdir/”) and came across this problem. I’ll submit a PR.

#include <glog/logging.h>

int main(int argc, char *argv[]) {
  google::InitGoogleLogging(argv[0]);
  google::SetLogDestination(google::INFO, "testdir/");
  google::EnableLogCleaner(1);
  LOG(INFO) << "msg";
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants