build: enable -Wsign-conversion warnings and fix/silence them (OpenSSF)
#13489
Conversation
vszakats
changed the title
-Wsign-conversion warnings-Wsign-conversion warnings
vszakats
force-pushed
the
signwarn-10
branch
2 times, most recently
from
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?:
|
vszakats
changed the title
-Wsign-conversion warnings-Wsign-conversion warnings and fix/silence them all
vszakats
changed the title
-Wsign-conversion warnings and fix/silence them all-Wsign-conversion warnings and fix/silence them
vszakats
force-pushed
the
signwarn-10
branch
2 times, most recently
from
7364f36
to
98c57d8
Compare
vszakats
changed the title
-Wsign-conversion warnings and fix/silence them-Wsign-conversion warnings and fix/silence them (OpenSSF)
vszakats
force-pushed
the
signwarn-10
branch
2 times, most recently
from
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.
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-conversionwarnings, mostly by casts and type changes.-Wsign-conversionwarning forFD_SET()/FD_ISSET()macros.These trigger on some systems due to the macros' implementation.
-Wsign-conversionwarnings (OpenSSF).-Wsign-conversionan error (OpenSSF).Related fixes:
printfmasks.tests: switch unit tests to return→ tests: make the unit test result typeCURLcode.CURLcode#13600sin_family/sin6_familyasshort.ctx_option_ttype for openssl 1.1.xCURLcode/OSStatusmix-up.ParameterErrormix-up.Curl_hash_add.Follow-up to 3829759 #12489
Closes #13489