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

build: silence -Wsign-conversion in FD_SET()/FD_ISSET() #13896

Closed
wants to merge 8 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Jun 5, 2024

Silence toolchain bugs causing warnings in FD_SET()/FD_ISSET() calls.

Seen with older Cygwin/MSYS2 builds. Likely fixed on 2020-08-03 by:
https://cygwin.com/git/?p=newlib-cygwin.git;a=commitdiff;h=5717262b8ecfed0f7fab63e2c09c78991e36f9dd

Also seen on Alpine MUSL:

multi.c:1201:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion]
 1201 |         FD_SET(ps.sockets[i], read_fd_set);
      |         ^~~~~~
multi.c:1201:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion]
multi.c:1203:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion]
 1203 |         FD_SET(ps.sockets[i], write_fd_set);
      |         ^~~~~~
multi.c:1203:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion]

Ref: https://github.com/curl/curl/actions/runs/8867959370/job/24347027402?pr=13489#step:31:373

And on OmniOS:

example/direct_tcpip.c:251:9: warning: conversion to 'long unsigned int' from 'libssh2_socket_t' {aka 'int'} may change the sign of the result [-Wsign-conversion]
    251 |         FD_SET(forwardsock, &fds);
        |         ^~~~~~
example/direct_tcpip.c:251:9: warning: conversion to 'long unsigned int' from 'libssh2_socket_t' {aka 'int'} may change the sign of the result [-Wsign-conversion]
example/direct_tcpip.c:251:9: warning: conversion to 'long unsigned int' from 'long int' may change the sign of the result [-Wsign-conversion]
example/direct_tcpip.c:251:9: warning: conversion to 'long int' from 'long unsigned int' may change the sign of the result [-Wsign-conversion]

Ref: https://github.com/libssh2/libssh2/actions/runs/8854199687/job/24316762831#step:3:2021

Ref: 5b9955e #13501
Ref: 95a882d #12557
Cherry-picked from #13489
Closes #13896


The fix is ugly. Perhaps creating a macro for it would be nicer, and then putting the original ones on the checksrc blocklist. [macro will not work, wrapper function problematic to match types, and performance.]

Seen on Alpine MUSL.

```
multi.c:1201:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion]
 1201 |         FD_SET(ps.sockets[i], read_fd_set);
      |         ^~~~~~
multi.c:1201:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion]
multi.c:1203:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion]
 1203 |         FD_SET(ps.sockets[i], write_fd_set);
      |         ^~~~~~
multi.c:1203:9: error: conversion to 'long unsigned int' from 'curl_socket_t' {aka 'int'} may change the sign of the result [-Werror=sign-conversion]
```
Ref: https://github.com/curl/curl/actions/runs/8867959370/job/24347027402?pr=13489#step:31:373
@vszakats
Copy link
Member Author

vszakats commented Jun 6, 2024

Should this one remain or be deleted? It's a pattern already done in examples.

@bagder
Copy link
Member

bagder commented Jun 7, 2024

I think this is another sign-conversion warning that does not help us and where the fix makes the code harder to read.

@vszakats
Copy link
Member Author

vszakats commented Jun 7, 2024

What about the pre-existing suppression in the example sendrecv.c, and one more in the open PR #12980? Should those be deleted?

@vszakats
Copy link
Member Author

vszakats commented Jun 13, 2024

What about the pre-existing suppression in the example sendrecv.c, and one more in the open PR #12980? Should those be deleted?

Any guidance on that? Perhaps we want to keep doing this in examples because people with -Wconversion (which implies -Wsign-conversion) can bump into this warning?

@vszakats vszakats closed this Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants