-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: enable -Wsign-conversion
warnings and fix/silence them (OpenSSF)
#13489
base: master
Are you sure you want to change the base?
Conversation
-Wsign-conversion
warnings-Wsign-conversion
warnings
1faf8eb
to
b73157c
Compare
PR now passes all CI builds and tests. It did help finding small issues, but I haven't investigated all possible ones, only the trivial ones. About half of the patch is addressing To my eyes the resulting code better expresses what's happening and more clearly describes There is no exact number for the warnings fixed. There were about 450 in the first round and Of these, around 140 were silenced with the It's possible that warnings are still present in code not compiled in CI and/or build combinations Next steps?:
|
-Wsign-conversion
warnings-Wsign-conversion
warnings and fix/silence them all
-Wsign-conversion
warnings and fix/silence them all-Wsign-conversion
warnings and fix/silence them
7364f36
to
98c57d8
Compare
-Wsign-conversion
warnings and fix/silence them-Wsign-conversion
warnings and fix/silence them (OpenSSF)
8bf863f
to
4dd290d
Compare
Separate PRs for the major curl components:
The above are the same sub-commits as here, except two controlling the warning option, which is unique to this one. |
To match the type used in 'set.happy_eyeballs_timeout'. Ref: #13489
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just think that adding typecasts in hundreds of new places is not a big win. They just silence the warnings with code that is a tad bit harder to read, the same logic is still there.
The improvements done by changing variable types and prototypes are the interesting ones I think. A typecast is always a bit of a "I surrender" sign.
Documentation says it's `long`, but really is `unsigned long`. ``` vtls/openssl.c:3695:38: error: conversion to 'long unsigned int' from 'ctx_option_t' {aka 'long int'} may change the sign of the result [-Werror=sign-conversion] 3695 | SSL_CTX_set_options(octx->ssl_ctx, ctx_options); | ^~~~~~~~~~~ ``` Ref: https://github.com/curl/curl/actions/runs/8868596554/job/24348345926#step:31:490
temp: ``` C:/projects/curl/lib/vtls/x509asn1.c:961:12: error: cast from function call of type 'CURLcode' to non-matching type 'int' [-Werror=bad-function-cast] 961 | return (int)do_pubkey_field(data, certnum, "ecPublicKey", pubkey); | ^ ``` Ref: https://ci.appveyor.com/project/curlorg/curl/builds/49704256/job/hcietrd5e7qbu73c#L326
``` tool_getparam.c:2904:37: error: implicit conversion changes signedness: 'ParameterError' to 'int' [-Werror,-Wsign-conversion] ``` Ref: https://github.com/curl/curl/actions/runs/8861448063/job/24333320546?pr=13489#step:10:1257
-Wsign-conversion
warnings, mostly by casts and type changes.-Wsign-conversion
warning forFD_SET()
/FD_ISSET()
macros.These trigger on some systems due to the macros' implementation.
-Wsign-conversion
warnings (OpenSSF).-Wsign-conversion
an error (OpenSSF).Related fixes:
printf
masks.tests: switch unit tests to return→ tests: make the unit test result typeCURLcode
.CURLcode
#13600sin_family
/sin6_family
asshort
.ctx_option_t
type for openssl 1.1.xCURLcode
/OSStatus
mix-up.ParameterError
mix-up.Curl_hash_add
.Follow-up to 3829759 #12489
Closes #13489