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

Cast variable type based on format identifier #113

Merged
merged 9 commits into from
Nov 24, 2020

Conversation

yourslab
Copy link
Contributor

Only ever use base format specifiers and always cast to the standard C type

@yourslab yourslab force-pushed the fix-logging-warnings branch from d9f0bb3 to 9a69474 Compare November 23, 2020 19:44
source/core_mqtt_serializer.c Outdated Show resolved Hide resolved
source/core_mqtt_serializer.c Outdated Show resolved Hide resolved
source/core_mqtt_serializer.c Outdated Show resolved Hide resolved
source/core_mqtt_serializer.c Outdated Show resolved Hide resolved
muneebahmed10
muneebahmed10 previously approved these changes Nov 24, 2020
@@ -605,7 +605,7 @@ static int32_t sendPacket( MQTTContext_t * pContext,

if( bytesSent < 0 )
{
LogError( ( "Transport send failed. Error code=%d.", bytesSent ) );
LogError( ( "Transport send failed. Error code=%d.", ( int ) bytesSent ) );

Choose a reason for hiding this comment

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

Looks like bytesSent is an int32. For the particular case which started this, the message was
format '%d' expects argument of type 'int', but argument 4 has type 'int32_t' {aka 'long int'}

I believe we want to use %ld and cast to long to handle the case where int is smaller that 32 bits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I'll cast int32_t to long int and int16_t to int.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why cast int16_t to int? A short is guaranteed to be at least 2 bytes.

Nit: long int and long are identical, I think we've been leaving off the int in other places where it's not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah. I think I just wanted to err on the side of caution, but I didn't end up casting any int16_t types anyway.

Comment on lines 944 to 945
LogError( ( "Failed to update state of publish %u.",
( unsigned int ) packetId ) );
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is originally a uint16_t, should we use unsigned short?

Suggested change
LogError( ( "Failed to update state of publish %u.",
( unsigned int ) packetId ) );
LogError( ( "Failed to update state of publish %hu.",
( unsigned short ) packetId ) );

Copy link
Contributor Author

@yourslab yourslab Nov 24, 2020

Choose a reason for hiding this comment

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

I think the definition of short is really dependent on the compiler, so I wanted to err on the side of caution. Although, we use short in other places for 16-bit data types, so I have updated it.

Copy link
Contributor

Choose a reason for hiding this comment

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

An unsigned short is required to be able to hold values up to 65,535, according to 5.2.4.2

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.

4 participants