Skip to content

Fix sign error, int/size_t discrepancies, warnings on windows builds. #936

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
Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix sign error, int/size_t discrepancies, warnings on windows builds. #936

wants to merge 5 commits into from
+59 −46

Conversation

@kristjanvalur
Copy link
Contributor

@kristjanvalur kristjanvalur commented Apr 8, 2021 •
edited
Loading

A lot of discrepancies with int vs size_t were visible when building on windows.
This PR fixes those.

kristjanvalur
kristjanvalur commented Apr 8, 2021
View reviewed changes
hiredis.c
hi_free(curargv);
*target = cmd;
return totlen;
return (int)totlen;
Copy link
Contributor Author

@kristjanvalur kristjanvalur Apr 8, 2021

Choose a reason for hiding this comment

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

These commands really should return a ssize_t result to be consistent with the use of size_t, but I didn't want to modify their signature since they are a public API

Copy link
Contributor Author

@kristjanvalur kristjanvalur Jan 18, 2022

Choose a reason for hiding this comment

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

Since then, have updated to reflect the new api which returns long long. Why that was chosen instead of size_t or ssize_t is unclear.

kristjanvalur
kristjanvalur commented Apr 8, 2021
View reviewed changes
hiredis.c
hi_free(curargv);
*target = cmd;
return totlen;
return (int)totlen;
Copy link
Contributor Author

@kristjanvalur kristjanvalur Apr 8, 2021

Choose a reason for hiding this comment

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

These commands really should return a ssize_t result to be consistent with the use of size_t, but I didn't want to modify their signature since they are a public API

Copy link
Contributor Author

@kristjanvalur kristjanvalur Jan 18, 2022

Choose a reason for hiding this comment

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

since this comment was written, the API has been updated to return long long. Why that was chosen instead of the more standard ssize_t remains unclear.

kristjanvalur
kristjanvalur commented Apr 8, 2021
View reviewed changes
net.c

for (b = bservinfo; b != NULL; b = b->ai_next) {
if (bind(s,b->ai_addr,b->ai_addrlen) != -1) {
if (bind(s,b->ai_addr,(socklen_t)b->ai_addrlen) != -1) {
Copy link
Contributor Author

@kristjanvalur kristjanvalur Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Windows has ai_addren as size_t rather than socklen_t. explicitly cast it to silence warning.

kristjanvalur
kristjanvalur commented Apr 8, 2021
View reviewed changes
net.c

c->flags |= REDIS_CONNECTED;
return REDIS_OK;
oom:
Copy link
Contributor Author

@kristjanvalur kristjanvalur Apr 8, 2021

Choose a reason for hiding this comment

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

Fix error about unused label (windows)

kristjanvalur
kristjanvalur commented Apr 8, 2021
View reviewed changes
read.c
if (v > ((unsigned long long)(-(LLONG_MIN+1))+1)) /* Overflow. */
return REDIS_ERR;
if (value != NULL) *value = -v;
if (value != NULL) *value = -(long long)v;
Copy link
Contributor Author

@kristjanvalur kristjanvalur Apr 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

warning on windows. Negation of an unsigned variable is shaky ground in C, better to be explicit.

kristjanvalur
kristjanvalur commented Apr 8, 2021
View reviewed changes
read.c Outdated
obj = r->fn->createArray(cur,elements);
else
obj = (void*)(long)cur->type;
obj = (void*)(intptr_t)cur->type;
Copy link
Contributor Author

@kristjanvalur kristjanvalur Apr 8, 2021

Choose a reason for hiding this comment

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

Must use an integral type of the same size as pointer to avoid the (sensible) warning here.

@kristjanvalur kristjanvalur marked this pull request as ready for review April 8, 2021 16:50
@kristjanvalur kristjanvalur changed the title Fix int/size_t discrepancies, warnings on windows builds. Fix sign error, int/size_t discrepancies, warnings on windows builds. Apr 8, 2021
@kristjanvalur kristjanvalur force-pushed the pr6 branch 2 times, most recently from d518279 to f995a71 Compare April 20, 2021 17:47
@kristjanvalur
Copy link
Contributor Author

kristjanvalur commented Oct 13, 2021

Any news here?

@kristjanvalur
Copy link
Contributor Author

kristjanvalur commented Jan 18, 2022

In the meantime, there have been some slight fixes upstream. Curiosly, the authors have decided to use long long in places in stead of the more standard size_t for reasons unknown. It should be best to stick to ssize_t since this better reflects the memory model supported.

@kristjanvalur
Copy link
Contributor Author

kristjanvalur commented Jun 24, 2022

Would it be possible to get some sort of review on ths PR please?

@kristjanvalur
Copy link
Contributor Author

kristjanvalur commented Jun 30, 2022

This PR fixes a lot of internal type discrepancies within functions, using size_t and ssize_t where appropriate.
It does also contain inline comments such as

 return (long long) totlen; /* api really should use ssize_t */

I'll be happy to remove those, since they reflect my personal opinion :)

@kristjanvalur kristjanvalur force-pushed the pr6 branch 2 times, most recently from 83fb1a1 to d7341ca Compare July 7, 2022 11:09
@kristjanvalur
Copy link
Contributor Author

kristjanvalur commented Jul 7, 2022

I've squashed and rebased this on top of current main. This removes all size warnings from windows builds.
To test it, run something like this on windows:

mkdir win-build
cd win-build
'C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Auxiliary\Build\vcvarsall.bat' x86
cmake ..
cmake --build . --clean-first

and for amd64

del CMakeCache.txt
'C:\Program Files\Microsoft Visual Studio\2022\Professional\VC\Auxiliary\Build\vcvarsall.bat' amd64
cmake ..
cmake --build . --clean-first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@kristjanvalur