Replace magic numbers with macros and add explaining comments #249
Conversation
|
I've approved. I'll take it you're using the build checks for your testing? |
Yes, I relied on the PR unit-tests and coverage to validate that the code is correct. |
| @@ -2862,7 +2946,7 @@ MQTTStatus_t MQTT_Ping( MQTTContext_t * pContext ) | |||
| MQTTFixedBuffer_t localBuffer; | |||
There was a problem hiding this comment.
| MQTTFixedBuffer_t localBuffer; | |
| uint8_t pingreqPacket[ MQTT_PACKET_PINGREQ_SIZE ]; |
| @@ -2877,6 +2961,7 @@ MQTTStatus_t MQTT_Ping( MQTTContext_t * pContext ) | |||
|
|
|||
There was a problem hiding this comment.
Line above isnt necessary.
| @@ -2877,6 +2961,7 @@ MQTTStatus_t MQTT_Ping( MQTTContext_t * pContext ) | |||
|
|
|||
| if( status == MQTTSuccess ) | |||
| { | |||
| assert( packetSize == localBuffer.size ); | |||
There was a problem hiding this comment.
| assert( packetSize == localBuffer.size ); |
| @@ -2904,7 +2989,7 @@ MQTTStatus_t MQTT_Ping( MQTTContext_t * pContext ) | |||
| * from the user provided buffers. Thus it can be sent directly. */ | |||
| sendResult = sendBuffer( pContext, | |||
| localBuffer.pBuffer, | |||
| 2U ); | |||
| packetSize ); | |||
There was a problem hiding this comment.
| packetSize ); | |
| MQTT_PACKET_PINGREQ_SIZE ); |
| uint8_t * pIndex; | ||
| TransportOutVector_t pIoVector[ MQTT_SUB_UNSUB_MAX_VECTORS ]; | ||
| TransportOutVector_t * pIterator; | ||
| uint8_t serializedTopicFieldLength[ MQTT_SUB_UNSUB_MAX_VECTORS ][ 2 ]; |
There was a problem hiding this comment.
Seems like a 16-bit length type would be a simpler way to handle this?
| @@ -161,7 +256,7 @@ static int32_t sendMessageVector( MQTTContext_t * pContext, | |||
| * | |||
| * @return The number of vectors added. | |||
| */ | |||
| static size_t addEncodedStringToVector( uint8_t serailizedLength[ 2 ], | |||
There was a problem hiding this comment.
A typedef seems like a better choice here.
| size_t vectorsAdded = 0U; | ||
|
|
||
| /* When length is non-zero, the string must be non-NULL. */ | ||
| assert( ( length != 0U ) == ( string != NULL ) ); | ||
|
|
||
| serailizedLength[ 0 ] = ( ( uint8_t ) ( ( length ) >> 8 ) ); | ||
| serailizedLength[ 1 ] = ( ( uint8_t ) ( ( length ) & 0x00ffU ) ); | ||
| serializedLength[ 0 ] = ( ( uint8_t ) ( ( length ) >> 8 ) ); |
There was a problem hiding this comment.
A macro or static inline helper function could improve clarity here
source/core_mqtt.c
Outdated
| * Username + 2 = 9 | ||
| * Password + 2 = 11 | ||
| */ | ||
| #define CORE_MQTT_CONNECT_PACKET_MAX_VECTORS ( 11U ) |
There was a problem hiding this comment.
Similar situation here. This is only used once.
Description
This PR removes the magic numbers from the code and replaces them with macros.
The value to which the macro is set is explained with comments in Doxygen formatting.
Test Steps
Checklist:
Related Issue
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.