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 #398

Closed
wants to merge 1 commit into from

Conversation

dzbaker
Copy link
Contributor

@dzbaker dzbaker commented Jul 11, 2023

Checklist

Describe the contribution
Fixes #288
CF_UnionArgs_Payload_t has been removed, given that only a single member of the 3 is used in CF. That member variable - byte - has been moved into the CF_UnionArgsCmd_t struct, which was the only place where CF_UnionArgs_Payload_t was used.
Re-introduced from #341. #393, #395 reverted this PR.

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

Expected behavior changes
No impact on logic.

Contributor Info
@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.
@dzbaker
Copy link
Contributor Author

dzbaker commented Jul 11, 2023

@thnkslprpt I reverted the original (#341) so we could move it to the next release. Does everything look good here?

@thnkslprpt
Copy link
Contributor

@thnkslprpt I reverted the original (#341) so we could move it to the next release. Does everything look good here?

@dzbaker I'm unsure of the effects of this PR as it includes a few other merged commits on top of the original commit removing the CF_UnionArgs_Payload_t union.

It's quite a simple change - maybe easier if I submit a new PR just going from the current main branch with just the removal of CF_UnionArgs_Payload_t?

@dzbaker
Copy link
Contributor Author

dzbaker commented Jul 12, 2023

@thnkslprpt I reverted the original (#341) so we could move it to the next release. Does everything look good here?

@dzbaker I'm unsure of the effects of this PR as it includes a few other merged commits on top of the original commit removing the CF_UnionArgs_Payload_t union.

It's quite a simple change - maybe easier if I submit a new PR just going from the current main branch with just the removal of CF_UnionArgs_Payload_t?

@thnkslprpt Yes, thank you, it may be best to submit a new PR and I can close this one.

@thnkslprpt
Copy link
Contributor

@thnkslprpt Yes, thank you, it may be best to submit a new PR and I can close this one.

Cool no worries.
Done (#400)

@dzbaker
Copy link
Contributor Author

dzbaker commented Jul 12, 2023

Replaced by #400.

@dzbaker dzbaker closed this Jul 12, 2023
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
2 participants