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 comparison of integer expressions of different signedness #403

Merged
merged 1 commit into from
Jan 5, 2022

Conversation

rleh
Copy link
Contributor

@rleh rleh commented Dec 31, 2021

Fix comparison of integer expressions of different signedness

Description

When compiling with -Werror=sign-compare flag and with the default config values (FreeRTOSIPConfigDefaults.h) the integer comparison of different signedness in FreeRTOS_TCP_WIN.c:1874 leads to an error, because winSRTT_CAP_mS evaluates to 50U.
This PR removes the unsigned literal.

Test Steps

Compile with -Werror=sign-compare flag and without a user-defined value for ipconfigTCP_SRTT_MINIMUM_VALUE_MS.

Related Issue

FreeRTOS_TCP_WIN.c: In function 'prvTCPWindowTxCheckAck_CalcSRTT':
FreeRTOS_TCP_WIN.c:1874:33: error: comparison of integer expressions of different signedness: 'int32_t' {aka 'long int'} and 'unsigned int' [-Werror=sign-compare]
 1874 |             if( pxWindow->lSRTT < winSRTT_CAP_mS )
      |                                 ^

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice. ✅

@rleh rleh requested a review from a team as a code owner December 31, 2021 13:32
@rleh rleh mentioned this pull request Dec 31, 2021
15 tasks
Copy link
Contributor

@htibosch htibosch left a comment

Choose a reason for hiding this comment

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

Thank you Raphael for this PR.
I approve it.
It may take a while before it will be approved and merged.

@@ -380,7 +380,7 @@
* The default has always been 50, but a value of 1000
* is recommended (see RFC6298) because hosts often delay the
* sending of ACK packets with 200 ms. */
#define ipconfigTCP_SRTT_MINIMUM_VALUE_MS 50U
#define ipconfigTCP_SRTT_MINIMUM_VALUE_MS 50
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes correct, as the value will be compared with an int32_t

Copy link
Member

Choose a reason for hiding this comment

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

@rleh thanks for this PR.

For short term I think this is okay, but since milliseconds cannot be negative, I think that the macro should be reverted back to the original value of 50U and the variable should be modified to be uint32_t if it only ever holds milliseconds.

@alfred2g alfred2g merged commit c33a9ac into FreeRTOS:main Jan 5, 2022
@rleh rleh deleted the fix/intger_signedness branch January 5, 2022 13:40
alfred2g pushed a commit to alfred2g/FreeRTOS-Plus-TCP that referenced this pull request Jan 31, 2022
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.

5 participants