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

Fix #288, Remove unnecessary CF_UnionArgs_Payload_t union #400

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thnkslprpt
Copy link
Contributor

Checklist

Describe the contribution

Testing performed
GitHub CI actions (incl. Build + Run, Unit Tests etc.) all passing successfully.

Expected behavior changes
No impact on logic.
Code is simplified and clearer.

Contributor Info
Avi Weiss @thnkslprpt

{
ret = fn(cmd->data.byte[0], context);
ret = fn(cmd->byte[0], context);

Check notice

Code scanning / CodeQL

Use of non-constant function pointer Note

This call does not go through a const function pointer.
@thnkslprpt thnkslprpt force-pushed the redo-fix-288-remove-CF_UnionArgs_Payload_t-union branch from 0dd858e to 6cd5bd0 Compare October 26, 2023 12:45
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

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

We need to also do this but I'd like to approach it a bit differently. Although only the "byte" field is used now, it is still used for a few different things in different commands. Now that each command has its own definition, this should be made type-correct.

@thnkslprpt
Copy link
Contributor Author

We need to also do this but I'd like to approach it a bit differently. Although only the "byte" field is used now, it is still used for a few different things in different commands. Now that each command has its own definition, this should be made type-correct.

@jphickey So you want to create command-specific struct members for each command?
What exactly do you mean in terms of the solution to make it type-correct?
Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CF_UnionArgs_Payload_t elements dword and hword not used, union not necessary
3 participants