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
journal: Trivial cleanup #19317
journal: Trivial cleanup #19317
Conversation
src/journal/Journaler.cc
Outdated
assert(m_trimmer == nullptr); | ||
assert(m_player == nullptr); | ||
assert(m_recorder == nullptr); | ||
assert(!m_trimmer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I prefer the original way (i.e. don't treat non-booleans as booleans).
src/journal/Journaler.cc
Outdated
@@ -231,6 +237,7 @@ void Journaler::create(uint8_t order, uint8_t splay_width, | |||
int r = m_header_ioctx.aio_operate(m_header_oid, comp, &op); | |||
assert(r == 0); | |||
comp->release(); | |||
comp = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: it's a local variable going out-of-scope immediately
src/journal/Journaler.cc
Outdated
os << *journaler.m_metadata; | ||
} else { | ||
os << "NULL"; | ||
os << "nullptr"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: find and replace fail?
a721fbe
to
dbefacc
Compare
@dillaman Please check, when you get a chance. |
4ac5c12
to
9c26f7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
src/journal/Journaler.cc
Outdated
os << *journaler.m_metadata; | ||
} else { | ||
os << "NULL"; | ||
os << "failed to find and replace"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: in my original comment, I was attempting to say no need to change this line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dillaman We should say something here which is more helpful for users, shouldn't we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure -- you can say "uninitialized"
Signed-off-by: Shinobu Kinjo <shinobu@redhat.com>
9c26f7e
to
fb50bde
Compare
@dillaman finally i noticed you're absolutely right. i deleted such a stupid change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Signed-off-by: Shinobu Kinjo shinobu@redhat.com