Skip to content

Improve FunctionBar code regarding size handling #1720

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Explorer09
Copy link
Contributor

@Explorer09 Explorer09 commented Jun 8, 2025

  • Demote the FunctionBar.size member from uint32_t to unsigned int type because the value is never meant to be greater than FUNCTIONBAR_MAXEVENTS (== 15).
  • (Cherry-picked to main) Add an assertion in FunctionBar_new() to indicate the fact above.
  • (Cherry-picked to main) Use size_t for loop iterators (array indices) in all FunctionBar methods. (This change will depend on compiler optimization to keep the same code size. In particular the recognition that a smaller iterator (32-bit) can be used when the limit is constrained to 32-bit integers.)

@BenBE
Copy link
Member

BenBE commented Jun 8, 2025

The use of uint32_t was intentional to have a minimum of size guarantees for the structure layout.

@BenBE BenBE added the code quality ♻️ Code quality enhancement label Jun 8, 2025
@Explorer09
Copy link
Contributor Author

The use of uint32_t was intentional to have a minimum of size guarantees for the structure layout.

I can't get it. What I see is the value would never exceed FUNCTIONBAR_MAXEVENTS and it would not be necessary to use uint32_t. Given that unsigned int is also 32 bits in many processor architectures, the change to unsigned int shouldn't affect structure size.

To prevent further problems with structure alignment (what were addressed in #1702), I moved non-pointer members (size and staticData) to after the pointer members.

@BenBE
Copy link
Member

BenBE commented Jun 9, 2025

The other option, which I chose was to just fix the size of members so alignment gets resolved that way. Also, I prefer to have integer sizes fixed, unless the API requires otherwise …

I think we can skip the additional shuffling of members and if you just keep the uint32_t we're good to go …

@Explorer09
Copy link
Contributor Author

I've split the changes of this PR into two commits, so that the part without dispute can be applied (cherry-picked) first.

And I'm still not convinced with keeping the uint32_t type anyway.

@BenBE BenBE force-pushed the functionbar-size branch from 2ce0d3e to a4c87b1 Compare June 15, 2025 20:37
To prevent future problem with structure field alignment, move the
non-pointer members ('size' and 'staticData') to the end of the
FunctionBar structure.
The value of the 'FunctionBar.size' member is never meant to be greater
than FUNCTIONBAR_MAXEVENTS (== 15) and so 'unsigned int' would suffice.
No need to use a fixed 32-bit integer type.

Signed-off-by: Kang-Che Sung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality ♻️ Code quality enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants