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

Fix conversion #7520

Merged
merged 8 commits into from
May 14, 2024
Merged

Fix conversion #7520

merged 8 commits into from
May 14, 2024

Conversation

bandi13
Copy link
Contributor

@bandi13 bandi13 commented May 10, 2024

Fix issues related to -Wconversion and -Wsign-conversion. Most notably where a variable needs to be cast to a different type. Not including implicit type upgrades that happen in equations (ie: uint8 + uint16).

@bandi13 bandi13 self-assigned this May 10, 2024
@bandi13 bandi13 force-pushed the fixConversion branch 7 times, most recently from d72da4f to 647ab96 Compare May 14, 2024 14:41
@dgarske
Copy link
Contributor

dgarske commented May 14, 2024

@bandi13 , seems like: ./configure CFLAGS="-Wconversion -Wsign-conversion" does a good job reproducing the warnings. This PR does not resolve them all, should I expect a certain section to be resolved? I believe you are planning to split up the work into multiple PR's. Do we really care about resolving these warnings outside the library proper (like API unit test and examples)? Seems like size_t is used enough places that we can assume it's available, right? Looks like it comes from stddef.h.

Copy link
Contributor

@dgarske dgarske left a comment

Choose a reason for hiding this comment

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

Lots of great cleanups for implicit casts. I don't foresee any portability issues. Will wait to merge until your response on my comment.

@bandi13
Copy link
Contributor Author

bandi13 commented May 14, 2024

Oh for sure there are a ton more to do. I was basically going through by type of error, not based on file the error occurs in. At some point I ended up scripting some of the changes.

Ultimately, I'd like to have -Wconversion tests enabled on PRs so we don't get into this mess again. It doesn't take very long to run, and it could run as a GitHub Action.

byte *data;

(void)heap;
/* okmLen (2) + protocol|label len (1) + info len(1) + protocollen +
* labellen + infolen */
len = 4 + protocolLen + labelLen + infoLen;
len = (size_t)4 + protocolLen + labelLen + infoLen;
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to solve with 4U not cast to make unsigned.

@@ -1690,7 +1690,7 @@ const char* wolfSSL_get_shared_ciphers(WOLFSSL* ssl, char* buf, int len)
return NULL;

cipher = wolfSSL_get_cipher_name_iana(ssl);
len = min(len, (int)(XSTRLEN(cipher) + 1));
len = (int)min((word32)len, (int)(XSTRLEN(cipher) + 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

Our MIN() uses word32, however I'm not sure that is true for all cases. But for our testing purposes this should be fine.

@dgarske dgarske merged commit 28bd4eb into wolfSSL:master May 14, 2024
103 checks passed
@bandi13 bandi13 deleted the fixConversion branch May 14, 2024 20:02
jefferyq2 pushed a commit to jefferyq2/wolfssl that referenced this pull request Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants