Fix sign error, int/size_t discrepancies, warnings on windows builds. by kristjanvalur · Pull Request #936 · redis/hiredis

kristjanvalur · 2021-04-08T16:44:09Z

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

kristjanvalur · 2021-04-08T16:45:11Z

hiredis.c

    hi_free(curargv);
    *target = cmd;
-    return totlen;
+    return (int)totlen;


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

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 · 2021-04-08T16:45:21Z

hiredis.c

    hi_free(curargv);
    *target = cmd;
-    return totlen;
+    return (int)totlen;


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

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 · 2021-04-08T16:45:56Z

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) {


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

kristjanvalur · 2021-04-08T16:46:35Z

net.c


    c->flags |= REDIS_CONNECTED;
    return REDIS_OK;
+oom:


Fix error about unused label (windows)

kristjanvalur · 2021-04-08T16:47:49Z

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;


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

kristjanvalur · 2021-04-08T16:49:24Z

read.c

                obj = r->fn->createArray(cur,elements);
            else
-                obj = (void*)(long)cur->type;
+                obj = (void*)(intptr_t)cur->type;


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

kristjanvalur · 2021-10-13T20:19:47Z

Any news here?

kristjanvalur · 2022-01-18T12:09:01Z

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 · 2022-06-24T17:15:21Z

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

kristjanvalur · 2022-06-30T10:37:18Z

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 · 2022-07-07T11:10:16Z

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

kristjanvalur commented Apr 8, 2021

View reviewed changes

net.c

c->flags |= REDIS_CONNECTED;

return REDIS_OK;

oom:

Copy link
Copy Markdown

Contributor Author

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 commented Apr 8, 2021

View reviewed changes

kristjanvalur marked this pull request as ready for review April 8, 2021 16:50

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 force-pushed the pr6 branch 2 times, most recently from d518279 to f995a71 Compare April 20, 2021 17:47

kristjanvalur force-pushed the pr6 branch from c815f6f to 5878130 Compare March 22, 2022 13:20

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

Bugfix: Fix incorrect negation of an unsigned value

e10388c

kristjanvalur force-pushed the pr6 branch from d7341ca to e724257 Compare July 7, 2022 11:15

kristjanvalur added 4 commits July 7, 2022 11:16

Fix various int/size_t discrepancies reported by microsoft C

429c07f

Disable windows warnings for strdup(), etc.

4c8e732

use redisFD type

5e8900a

fix call to sdscatfmt

b4b858d

kristjanvalur force-pushed the pr6 branch from e724257 to b4b858d Compare July 7, 2022 11:17

Conversation

kristjanvalur commented Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kristjanvalur Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

kristjanvalur Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

kristjanvalur Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

kristjanvalur Jan 18, 2022

Choose a reason for hiding this comment

Uh oh!

kristjanvalur Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristjanvalur Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

kristjanvalur Apr 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kristjanvalur Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

kristjanvalur commented Oct 13, 2021

Uh oh!

kristjanvalur commented Jan 18, 2022

Uh oh!

kristjanvalur commented Jun 24, 2022

Uh oh!

kristjanvalur commented Jun 30, 2022

Uh oh!

kristjanvalur commented Jul 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kristjanvalur commented Apr 8, 2021 •

edited

Loading

kristjanvalur Apr 8, 2021 •

edited

Loading

kristjanvalur Apr 8, 2021 •

edited

Loading