Fix MISRA C 2012 Rule 13.3 Violations - #988
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #988 +/- ##
=======================================
Coverage 93.53% 93.53%
=======================================
Files 6 6
Lines 3200 3200
Branches 889 889
=======================================
Hits 2993 2993
Misses 92 92
Partials 115 115
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| \ | ||
| ( pxItemToRemove )->pxContainer = NULL; \ | ||
| ( pxList->uxNumberOfItems )--; \ | ||
| pxList->uxNumberOfItems -= 1U; \ |
There was a problem hiding this comment.
pxList->uxNumberOfItems has the type UBaseType_t . This operation leads to -Wconversion error , conversion from ‘unsigned int’ to ‘UBaseType_t’ . It can be changed to
pxList->uxNumberOfItems = ( UBaseType_t )( pxList->uxNumberOfItems - 1U );
| pxDelayedTaskList = pxOverflowDelayedTaskList; \ | ||
| pxOverflowDelayedTaskList = pxTemp; \ | ||
| xNumOfOverflows++; \ | ||
| xNumOfOverflows += 1; \ |
There was a problem hiding this comment.
xNumOfOverflows has the type BaseType_t . This operation leads to -Wconversion error , conversion from ‘unsigned int’ to ‘BaseType_t’ . It can be changed to
xNumOfOverflows = ( BaseType_t )( xNumOfOverflows + 1U );
There was a problem hiding this comment.
I am happy to change it to this implementation, but went with leaving the compound assignment operators in and casting the value on the right side of the operator to be the appropriate type as the C standard states they differ in that "A compound assignment of the form E1 op= E2 differs from the simple assignment expression E1 = E1 op (E2) only in that the lvalue E1 is evaluated only once."
There was a problem hiding this comment.
I am also curious here why this variable is volatile, because if it can change (and the post-increment causes a rmw race) then doing the +=1 will create the exact same race right?
| /* The scheduler is suspended if uxSchedulerSuspended is non-zero. An increment | ||
| * is used to allow calls to vTaskSuspendAll() to nest. */ | ||
| ++uxSchedulerSuspended; | ||
| uxSchedulerSuspended += 1U; |
There was a problem hiding this comment.
uxSchedulerSuspended has the type UBaseType_t . This operation leads to -Wconversion error , conversion from ‘unsigned int’ to ‘UBaseType_t’ .
There was a problem hiding this comment.
I am curious why this used to be a pre-increment, that seems slightly odd, at least unexpected in this case.
There was a problem hiding this comment.
And of course like the other one I am curious why this variable is volatile, because if it is indeed volatile then the read/modify/write we are doing here creates a potential race condition where we read it, then add 1, then the value gets incremented from an ISR e.g. , and then we overwrite with an incorrect value here ?
|
/bot run formatting |
6068d7e to
a707cf8
Compare
| \ | ||
| ( pxItemToRemove )->pxContainer = NULL; \ | ||
| ( pxList->uxNumberOfItems )--; \ | ||
| ( ( pxList )->uxNumberOfItems ) -= ( UBaseType_t ) 1U; \ |
There was a problem hiding this comment.
One simplification:
- ( ( pxList )->uxNumberOfItems ) -= ( UBaseType_t ) 1U;
+ ( ( pxList )->uxNumberOfItems ) -= 1U; I think that the cast is not necessary. Have a try.
Also try:
- xNumOfOverflows += ( BaseType_t ) 1;
+ xNumOfOverflows += 1; EDIT Ok, reading back I see that a compiler doesn't agree with an implicit cast from 1U to UBaseType_t. That's a pity.
There was a problem hiding this comment.
The problem is that BaseType_t can be a different size from "unsigned", which means this will work fine with some types as BaseType_t but not others. These cases are tricky because compiling only with one type for BaseType_t will not expose all the problems !
|
This reverts commit 4d34700.
Description
Rule 13.3 states that a full expression containing the increment (++) or decrement (--) operator should have no other potential side effects other than that caused by the increment or decrement operator. These were flagged as violations of the rule since the read of a volatile variable is considered to be a side effect in itself. As a result, using
+= 1and-= 1in place of++and--brings these lines into compliance with the rule.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.