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

No alloc #34

Closed
wants to merge 16 commits into from
Closed

No alloc #34

wants to merge 16 commits into from

Conversation

mriise
Copy link
Contributor

@mriise mriise commented Nov 18, 2020

Low effort no_alloc, still missing tests and public methods for the crate like multibase::decode_mut().

Also has a few minor changes to README.

fixes #34

@mriise
Copy link
Contributor Author

mriise commented Nov 18, 2020

CI for no_std needs to be updated as well, as no default features implies no_alloc

And I meant to open this as a draft, whoops

@mriise mriise requested a review from vmx November 18, 2020 17:44
@mriise mriise marked this pull request as draft November 19, 2020 01:39
Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

Thanks. It looks good so far!

@mriise
Copy link
Contributor Author

mriise commented Nov 22, 2020

@vmx Errors are handled by a custom result type, which doesn't play well with no_alloc as it implies from std::Error which currently requires an allocator we could try moving to error enums but that would make it more brittle. Other crates don't offer great options either for this sort of thing and the community discussion around it is decently heated and currently inconclusive.

pyfisch/cbor#79
rust-lang/rust#62502
rust-lang/rust#64017
rust-lang/rust#53487

@mriise
Copy link
Contributor Author

mriise commented Nov 22, 2020

After looking into implementing decode_mut etc for the Base enum, i've stumbled across the pains of splitting the API in half with mut. Since the error issue would be the next most pressing problem here, I think ill focus my efforts on getting base-x no_alloc compatible. It's more work than if I were to just hack the macro to only build the desired Codes into the enum, but It's work that would be beneficial as a whole.

@vmx
Copy link
Member

vmx commented Nov 23, 2020

@mriise I don't fully understand the problem with the errors. The current code has an Error enum and it is only implementing std::error::Error, if the std feature is enabled. What am I missing?

@mriise
Copy link
Contributor Author

mriise commented Nov 23, 2020

@vmx nothing actually, I was the one who was missing things. I just need to add the extra error types from data_encoding to the enum.

@mriise
Copy link
Contributor Author

mriise commented Jan 20, 2021

In retrospect I should've ran clippy myself instead of using build server time...

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

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

New changes came in while I was reviewing, so some things might not apply anymore.

src/impls.rs Outdated Show resolved Hide resolved
src/impls.rs Outdated Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
src/impls.rs Outdated

fn encode_mut<I: AsRef<[u8]>>(input: I, output: &mut [u8]){
let out = base_x::encode($encoding.0, input.as_ref());
output.copy_from_slice(out.as_bytes());
Copy link
Member

Choose a reason for hiding this comment

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

This could panic in case the sliced differ in size. This either needs to be documented or we check for the size. I lean towards checking for the size and returning an error if it doesn't match. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New error type MismatchedSizes or just the plain old DecodeError?

Copy link
Member

Choose a reason for hiding this comment

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

MismatchSizes sounds good as a user of the library might want to deal with it in a special way, e.g. increasing the size of the slice and trying it again.

src/encoding.rs Outdated Show resolved Hide resolved
src/encoding.rs Outdated Show resolved Hide resolved
src/encoding.rs Outdated Show resolved Hide resolved
@mriise
Copy link
Contributor Author

mriise commented Jan 20, 2021

New changes came in while I was reviewing, so some things might not apply anymore.

Not much was changed, just formatting and removing the unneeded log10 method

@mriise
Copy link
Contributor Author

mriise commented Jan 20, 2021

Here's a personal list not peticularly including things from the review,

  • More generic errors and better error information (I was stumbling on this when working and is why I made those weird Errors)
  • It should be clear that decode/encode_mut for base-x are extremely lazy and not optimized. The only reason it's implemented for them at all is to make the code table generic over one type. I can't see anyone reaching for these anyway outside of a no-alloc env where base-x isn't even compiled. It's there for convenience, not because it is particularly useful.
  • rest of the small things from the review

Looks like one or two more rounds of review and this should be ready to go!
I'm still considering making base-x no_alloc compatible and have gotten decently far, but that will be in a later PR.

@mriise
Copy link
Contributor Author

mriise commented Jan 28, 2021

@vmx Im thinking that just re-exporting the data-encoding errors and using them would be the better option here, it's a lot better to conform less informative errors into more complex errors than it is to transform complex and informative errors into less informative ones.

I may try to copy the error format from data-encoding to base-x as it could definitely use it.

@vmx
Copy link
Member

vmx commented Jan 28, 2021

it's a lot better to conform less informative errors into more complex errors than it is to transform complex and informative errors into less informative ones.

I agree with that. I just don't want to leak internal dependencies to the public API. But if we wrap (i guess re-exporting is fine as well) those errors that's fine. I just don't want to force us to break our API in case we decide to use other libraries for base encoding in the future.

src/encoding.rs Show resolved Hide resolved
/// math comes from here https://github.com/bitcoin/bitcoin/blob/f1e2f2a85962c1664e4e55471061af0eaa798d40/src/base58.cpp#L48
pub(crate) fn calc_decoded_size(base: usize, input_byte_size: usize) -> usize {
f64::ceil(input_byte_size as f64 * (f64::log10(base as f64) / f64::log10(256.0)) + 1.0) as usize
// this shouldn't need an extra one or ceiling, something is wrong somewhere
Copy link
Member

Choose a reason for hiding this comment

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

Agreed. This should ideally be:

(input_byte_size as f64 * (f64::log2(base as f64) / 8.0))) as usize
// or
(input_byte_size as f64 / (8.0 / f64::log2(base as f64))) as usize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely agree, but for whatever reason it fails all test cases without the round up and added 1...

Just rounding up gives one short for base10 in the preserves_two_leading_zeroes test

decode len 11>11 base Base10
12
[0, 0, 121, 101, 115, 32, 109, 97, 110, 105, 32, 33]
[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0]
thread 'preserves_two_leading_zeroes' panicked at 'range end index 12 out of range for slice of length 11'

I will double check to make sure I am doing the tests correctly before I think about possible issues with base-x

Copy link
Member

Choose a reason for hiding this comment

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

It's good that we have that test :) Your code is using .len_utf8(), that should just be .len() as for base encoding it's really about the bits and bytes and not about utf8 characters.

@mriise
Copy link
Contributor Author

mriise commented Jul 4, 2022

Closing as this doesnt have any changes that are relevant anymore (new PR should reference this)

@mriise mriise closed this Jul 4, 2022
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