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.

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 warnings about implicit cast #1064

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TedLyngmo
Copy link

Added a few explicit casts:
unsigned char flags = (unsigned char)s[-1];

Signed-off-by: Ted Lyngmo [email protected]

TedLyngmo added a commit to TedLyngmo/redis-plus-plus that referenced this pull request Apr 28, 2022
Also added a few warning options to the build:
-Wshadow -Wconversion -Wcast-qual

-Wsign-conversion can also be added if the below pull request is approved:
redis/hiredis#1064

Signed-off-by: Ted Lyngmo <[email protected]>
TedLyngmo added a commit to TedLyngmo/redis-plus-plus that referenced this pull request Apr 28, 2022
Also added a few warning options to the build:
-Wshadow -Wconversion -Wcast-qual

-Wsign-conversion can also be added if the below pull request is approved:
redis/hiredis#1064

Signed-off-by: Ted Lyngmo <[email protected]>
@TedLyngmo
Copy link
Author

TedLyngmo commented Apr 29, 2022

I see it's failing in Ubuntu and CentOS, presumably because of the skipped tests? Any idea what could cause it to skip the tests?

#86 Does not return a reply when the command times out: SKIPPED
#118 Does not return a reply when the command times out: SKIPPED
#147 Does not return a reply when the command times out: SKIPPED
*** 0 TESTS FAILED ***
*** 3 TESTS SKIPPED ***
Error: Process completed with exit code 1.

When running make -j hiredis-test && ./test.sh on my Ubuntu (in WSL2) it doesn't skip any tests and it passes all tests.

TedLyngmo added a commit to TedLyngmo/redis-plus-plus that referenced this pull request May 1, 2022
Also added a few warning options to the build:
-Wshadow -Wconversion -Wcast-qual

-Wsign-conversion can also be added if the below pull request is approved:
redis/hiredis#1064

Signed-off-by: Ted Lyngmo <[email protected]>
TedLyngmo added a commit to TedLyngmo/redis-plus-plus that referenced this pull request May 1, 2022
Also added a few warning options to the build:
-Wshadow -Wconversion -Wcast-qual

-Wsign-conversion can also be added if the below pull request is approved:
redis/hiredis#1064

Signed-off-by: Ted Lyngmo <[email protected]>
Added a few explicit casts to sds.h:

unsigned char flags = (unsigned char)s[-1];

Signed-off-by: Ted Lyngmo <[email protected]>
@bjosv
Copy link
Contributor

bjosv commented May 4, 2022

Any idea what could cause it to skip the tests?

You might already have found the reason, but the command DEBUG SLEEP which is needed by these testcases was introduced in Redis version 2.2.12. Some testcases requires an already running redis and there might be an old pre-2.2.12 version running on those distros by default.

@TedLyngmo
Copy link
Author

Any idea what could cause it to skip the tests?

You might already have found the reason, but the command DEBUG SLEEP which is needed by these testcases was introduced in Redis version 2.2.12. Some testcases requires an already running redis and there might be an old pre-2.2.12 version running on those distros by default.

Oh, ok, so there is currently no way to get patches through this gate - or what can I do?
I have some more ideas for patches to be able to enable more warnings. Will it be possible to get them in somehow?

@michael-grunder
Copy link
Collaborator

michael-grunder commented May 4, 2022

I'm hesitant to merge changes to the sds library because this code is also in Redis itself so we try to minimise divergence.

How are you invoking the tests? Treating skip as failures is an option that you don't have to set.

Edit: Ahh I see, they are even being skipped in GitHub CI. We recently switched to using the official Redis package so perhaps something has changed. I need to investigate why they're being skipped

@TedLyngmo
Copy link
Author

I'm hesitant to merge changes to the sds library because this code is also in Redis itself so we try to minimise divergence.

I totally understand that. If I manage to get a PR through in Redis itself, will that eventually end up in hiredis? If so, I may start there instead.

We recently switched to using the official Redis package so perhaps something has changed. I need to investigate why they're being skipped

Great! Cheers! Oups, now the macOS test failed too. :-)

@michael-grunder
Copy link
Collaborator

If I manage to get a PR through in Redis itself, will that eventually end up in hiredis

Yes for sure

Great! Cheers! Oups, now the macOS test failed too

The problem is due to Redis 7.0.0 disabling the DEBUG command by default. The fix is pretty simple so should be up shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants