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.

Sign up for GitHub

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

Jump to bottom

Fix #67, Use size_t for 'size' variables #68

Merged
merged 1 commit into from
Jun 1, 2023
Merged

Fix #67, Use size_t for 'size' variables #68

merged 1 commit into from
Jun 1, 2023

Conversation

thnkslprpt
Copy link
Contributor

@thnkslprpt thnkslprpt commented Apr 10, 2023 •
edited
Loading

Checklist

Describe the contribution

  • Fixes Size variables should use the size_t type #67
    • Converted the obvious candidates for conversion to size_t (mostly local variables).
    • Updated the related local test variables to size_t as well for consistency.
    • All variables updated were already unsigned (uint8, uint16 and mostly uint32).

4 functions had parameters converted to size_t:
MM_Verify32Aligned() - uint32 Size
MM_Verify16Aligned() - uint32 Size
MM_VerifyLoadDumpParams() - uint32 SizeInBytes
MM_VerifyPeekPokeParams() - uint8 SizeInBits

Even if using size_t is unnecessary in some cases (and wastes some space), it is more expressive and more compliant with the relevant coding standards.

Testing performed
GitHub CI actions all passing successfully.

Expected behavior changes
No change to behavior other than type changes outlined above.

Contributor Info
Avi Weiss @thnkslprpt

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 10, 2023
View reviewed changes
fsw/src/mm_utils.c
@@ -296,11 +296,11 @@
/* Verify load/dump memory parameters */
/* */
/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
bool MM_VerifyLoadDumpParams(cpuaddr Address, MM_MemType_t MemType, uint32 SizeInBytes, uint8 VerifyType)
bool MM_VerifyLoadDumpParams(cpuaddr Address, MM_MemType_t MemType, size_t SizeInBytes, uint8 VerifyType)

Check notice

Code scanning / CodeQL

Long function without assertion

All functions of more than 10 lines should have at least one assertion.
fsw/src/mm_utils.c
@@ -296,11 +296,11 @@
/* Verify load/dump memory parameters */
/* */
/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
bool MM_VerifyLoadDumpParams(cpuaddr Address, MM_MemType_t MemType, uint32 SizeInBytes, uint8 VerifyType)
bool MM_VerifyLoadDumpParams(cpuaddr Address, MM_MemType_t MemType, size_t SizeInBytes, uint8 VerifyType)

Check notice

Code scanning / CodeQL

Function too long

MM_VerifyLoadDumpParams has too many lines (173, while 60 are allowed).
fsw/src/mm_utils.c
@@ -134,11 +134,11 @@
/* Verify peek and poke command parameters */
/* */
/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
bool MM_VerifyPeekPokeParams(cpuaddr Address, MM_MemType_t MemType, uint8 SizeInBits)
bool MM_VerifyPeekPokeParams(cpuaddr Address, MM_MemType_t MemType, size_t SizeInBits)

Check notice

Code scanning / CodeQL

Function too long

MM_VerifyPeekPokeParams has too many lines (156, while 60 are allowed).
fsw/src/mm_utils.c
@@ -134,11 +134,11 @@
/* Verify peek and poke command parameters */
/* */
/* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * */
bool MM_VerifyPeekPokeParams(cpuaddr Address, MM_MemType_t MemType, uint8 SizeInBits)
bool MM_VerifyPeekPokeParams(cpuaddr Address, MM_MemType_t MemType, size_t SizeInBits)

Check notice

Code scanning / CodeQL

Long function without assertion

All functions of more than 10 lines should have at least one assertion.
@thnkslprpt thnkslprpt force-pushed the fix-67-use-size_t-for-size-variables branch from fe0963c to 5da3798 Compare April 22, 2023 09:02
@thnkslprpt thnkslprpt force-pushed the fix-67-use-size_t-for-size-variables branch from 5da3798 to a0a0677 Compare May 17, 2023 01:40
@dzbaker dzbaker requested a review from chillfig May 18, 2023 18:39
chillfig
chillfig requested changes May 25, 2023
View reviewed changes
fsw/src/mm_dump.c
@@ -103,9 +103,9 @@ bool MM_PeekMem(const MM_PeekCmd_t *CmdPtr, cpuaddr SrcAddress)
uint16 WordValue = 0;
uint32 DWordValue = 0;
int32 PSP_Status = 0;
uint32 BytesProcessed = 0;
size_t BytesProcessed = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Later on line 168, this gets assigned to MM_AppData.HkPacket.BytesProcessed. What do you think about changing the prototype of MM_AppData.HkPacket.BytesProcessed (BytesProcessed of MM_HkPacket_t in file mm_msg.h) to align with this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes Justin - great addition.
I ran through and checked - that member only ever gets assigned to other size_t's or uint's.

@thnkslprpt thnkslprpt force-pushed the fix-67-use-size_t-for-size-variables branch 2 times, most recently from dea9c72 to ac73642 Compare May 25, 2023 20:40
@thnkslprpt thnkslprpt requested a review from chillfig May 25, 2023 20:41
@thnkslprpt thnkslprpt force-pushed the fix-67-use-size_t-for-size-variables branch from ac73642 to 3b93ea3 Compare May 25, 2023 20:56
@thnkslprpt thnkslprpt force-pushed the fix-67-use-size_t-for-size-variables branch from 3b93ea3 to 2c0f6e1 Compare May 25, 2023 20:58
chillfig
chillfig approved these changes May 31, 2023
View reviewed changes
Copy link
Contributor

@chillfig chillfig left a comment

Choose a reason for hiding this comment

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

Looks good to me!

fsw/inc/mm_msg.h
@@ -102,8 +101,7 @@ typedef struct
{
CFE_MSG_CommandHeader_t CmdHeader; /**< \brief Command header */

uint8 DataSize; /**< \brief Size of the data to be written */
uint8 Padding1[3]; /**< \brief Structure padding */
Copy link
Contributor

Choose a reason for hiding this comment

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

Padding omission looks appropriate given the new variable, DataSize.

fsw/inc/mm_msg.h
@@ -87,8 +87,7 @@ typedef struct
{
CFE_MSG_CommandHeader_t CmdHeader; /**< \brief Command header */

uint8 DataSize; /**< \brief Size of the data to be read */
uint8 Padding[3]; /**< \brief Structure padding */
Copy link
Contributor

Choose a reason for hiding this comment

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

Padding omission looks appropriate given the new variable, DataSize.

@dzbaker dzbaker merged commit e76c2d3 into nasa:main Jun 1, 2023
@thnkslprpt thnkslprpt deleted the fix-67-use-size_t-for-size-variables branch June 1, 2023 21:57
@dmknutsen dmknutsen added this to the Equuleus milestone Aug 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chillfig chillfig chillfig approved these changes

Assignees
No one assigned
Labels
CCB:Approved enhancement Equuleus-rc1
Projects
None yet
Milestone
Equuleus
Development

Successfully merging this pull request may close these issues.

Size variables should use the size_t type
4 participants
@thnkslprpt @chillfig @dzbaker @dmknutsen