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

Make NotifyFunction Optional #42

Merged
merged 1 commit into from Jul 13, 2022
Merged

Make NotifyFunction Optional #42

merged 1 commit into from Jul 13, 2022

Conversation

Ayush1325
Copy link
Contributor

Rust uses Option<fn ..> to represent Nullable Function pointer. It will have the same exact ABI (according to the docs I linked above) as a fn .. but will allow null.

This is needed since the NotifyFunction parameter is Optional.

It might be a good idea to check what other Function pointers should be represented in this way.

Signed-off-by: Ayush Singh ayushsingh1325@gmail.com

Rust uses Option<fn ..> to represent Nullable Function pointer.

Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>
@dvdhrm
Copy link
Member

dvdhrm commented Jul 13, 2022

It might be a good idea to check what other Function pointers should be represented in this way.

Correction: -should +must

I was not aware that rust does not allow NULL as function pointer, but obviously it does not. While you can still transmute the function call to void* and pass NULL, this is really ugly. So yeah, you are on to something, I think we have to convert them all.

I was shortly thinking of adjusting the type-definition to include the Option, but I think the correct call is to adjust the protocol-functions, just as you did. It does make us vulnerable to future spec updates that somehow allow passing NULL were it didn't before, but lets not think too hard about that.

@dvdhrm dvdhrm merged commit f224cca into r-efi:main Jul 13, 2022
@Ayush1325 Ayush1325 deleted the option-fn branch July 14, 2022 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants