-
Notifications
You must be signed in to change notification settings - Fork 137
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
cherry-picking from #114 Pvs analyze part 2 #215
Conversation
- use nullptr instead of NULL - uniform initialisation
Use curly brace initialization style in libgearman-server/struct/gearmand.h lines 53-56? |
server_thread{}, | ||
id{}, | ||
lock{} | ||
//TODO uniform initialisation doesn't help with wakeup_event |
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.
What's this TODO?
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.
Unfortunately I can't remember.
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.
Probably should remove it then.
libgearman-server/struct/gearmand.h
Outdated
@@ -75,7 +74,7 @@ struct gearmand_st | |||
|
|||
void keepalive_idle(int keepalive_idle_) | |||
{ | |||
_keepalive= true; | |||
keepalive(1); |
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.
Is _keepalive= true
bad coding style according to PVS? It should be more efficient than keepalive(1)
unless the compiler can optimize it.
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.
the commit with the changes is removed
Should libgearman-server/struct/thread.h line 64 have curly braces after |
- use nullptr instead of NULL - uniform initialisation
- uniform initialisation - nullptr
277fff5
to
d34fd50
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.
Is there something we can turn on after these changes to make sure we don't backslide?
I guess the compiler disliked |
Improving of test coverage could help, I think. |
please, review.