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 param of Type::from_xdr_base64 #180

Merged
merged 1 commit into from
Nov 13, 2023
Merged

Fix param of Type::from_xdr_base64 #180

merged 1 commit into from
Nov 13, 2023

Conversation

leighmcculloch
Copy link
Member

@leighmcculloch leighmcculloch commented Nov 11, 2023

What

Change Type::from_xdr_base64 fn to accept any type that as-refs to [u8] instead of accepting a String.

Why

There's three reasons.

The first is that by accepting a String we're requiring the caller to provide an owned value to the function. The function doesn't need an owned value, so the request for an owned value is unnecessary.

The second is that by accepting a String we're requiring that the caller to provide a value that is stored in memory allocated by the global allocator (the heap). Again this is unnecessary.

The third is that the base64 may not be stored in a string, it might have been communicated over a network or some other medium where we've got it in bytes, and converting it to a string before passing it to the decoder is unnecessary work that has fallible cases to handle because Rust strs are required to be UTF-8.

There is no cost introduced with this change. The same approach on other types is generated in this way and it appears to be a mistake we didn't do it the same way here.

@leighmcculloch leighmcculloch marked this pull request as ready for review November 11, 2023 07:42
@leighmcculloch leighmcculloch merged commit 70a8851 into master Nov 13, 2023
3 checks passed
@leighmcculloch leighmcculloch deleted the fixparam branch November 13, 2023 17:35
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.

2 participants