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.

Sign up for GitHub

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

Jump to bottom

Fix a typo in b6a052f. #1190

Merged
merged 1 commit into from May 30, 2023
Merged

Fix a typo in b6a052f. #1190

merged 1 commit into from May 30, 2023

Conversation

yossigo
Copy link
Member

@yossigo yossigo commented May 30, 2023

No description provided.

@michael-grunder
Copy link
Collaborator

Thanks!

I love this because I looked at all those lines and didn't see it.

@michael-grunder michael-grunder merged commit 6de326e into redis:master May 30, 2023
8 of 11 checks passed
@michael-grunder michael-grunder mentioned this pull request May 30, 2023
Merged
@bjosv
Copy link
Contributor

bjosv commented May 31, 2023

The CI step that builds with CMake is using -Werror, but lacks some warning-options that is included in the Makefile.
The CI step that builds using the Makefile finds this issue due to more warning-flags, but didn't fail due to the lack of -Werror.
Maybe we should add -Werror to the CI-steps that builds using the Makefile too?

An alternative is update both CMakeLists and Makefile to be similar regarding to warnings (and if warning is an error?), but this might need special handling in the CMakeLists for different compilers, like MSVC (and/or platforms?).

zuiderkwast
zuiderkwast reviewed May 31, 2023
View reviewed changes
net.c
Comment on lines -239 to 242
if (res == -1); {
if (res == -1) {
__redisSetErrorFromErrno(c,REDIS_ERR_IO,"setsockopt(TCP_USER_TIMEOUT)");
redisNetClose(c);
return REDIS_ERR;
Copy link
Contributor

Choose a reason for hiding this comment

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

What! That's a disaster. :) It would always return REDIS_ERR.

I should have used this in a test. It would have caught it.

Sorry for this and well spotted @yossigo.

@yossigo yossigo mentioned this pull request Jun 1, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zuiderkwast zuiderkwast zuiderkwast left review comments

@michael-grunder michael-grunder michael-grunder approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@yossigo @michael-grunder @bjosv @zuiderkwast