-
Notifications
You must be signed in to change notification settings - Fork 884
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
fix(stream): avoid using designated initializer #1247
Conversation
src/server/stream_family.cc
Outdated
.approx_trim = opts.trim_approx, | ||
.limit = opts.limit, | ||
}; | ||
streamAddTrimArgs add_args; |
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.
This is still uninitialized, you should add this -
streamAddTrimArgs add_args; | |
streamAddTrimArgs add_args = {0}; |
C++ is a bit weird sometimes ;)
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.
Ok, but I am unsure - doesn't C++ automatically set struct field to empty values?
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.
It's complicated... to be technical, the trivial default constructor leaves integer members uninitialized. Adding '= {0}' makes it into a zero initialization so you get all zeros instead of random stack memory.
(obligatory link to https://twitter.com/i/status/1004017362381795329)
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.
done :-)
Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
By the way, designated initializers are not part of the standard (until 20) 🤓, but are supported by most compilers. I actually wanted to leave a comment on this case, but checked other places and saw that we use them from time to time. |
Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@gmail.com>
Following the comment, a warning is popping up while building the
stream_family.cc
file.It is because a designated initializer is used to initialize a
streamAddTrimArgs
type variable. The-Wmissing-field-initializers
throws a warning if all the struct fields are not initialized while using a designated initializer.In our case, we don't need to set other fields as they are mainly used for redis's internal
xadd
implementation specific fields and are not used instreamTrim
function.