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

decode: use exact decoded length rather than estimation #227

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

Conversation

mina86
Copy link
Contributor

@mina86 mina86 commented Feb 10, 2023

Fixes: #210
Fixes: #212

@mina86
Copy link
Contributor Author

mina86 commented Feb 10, 2023

By the way, I’m beginning to think that the interface for the internal_decode should be completely changed. Specifically to something like:

fn internal_decode(
    dst: *mut [MaybeUninit<u8>],
    src: *const [u8],
) -> Result<(), DecodeError>;

With the following contract:

  • (*dst).len() / 3 == (*src).len() / 4,
  • (*dst).len() % 3 == (*src).len() % 4 == 0,
  • source has no padding bytes and
  • dst and src either do not overlap, or dst as *mut () == src as *const ().

This would help address the following issues:

@tgross35
Copy link

@mina86 any chance you are able to push this over the line? The author suggested in #210 (comment) that maybe this could be split up into smaller PRs.

By the way, I’m beginning to think that the interface for the internal_decode should be completely changed. Specifically to something like:

fn internal_decode(
    dst: *mut [MaybeUninit<u8>],
    src: *const [u8],
) -> Result<(), DecodeError>;

FYI this would need to be an unsafe function, and MaybeUninit doesn't add anything to integers (since all possible byte representations are valid integers)

@marshallpierce
Copy link
Owner

I'm quite happy to rearrange APIs to have nice new features, but unsafe is going to be a pretty hard sell. It adds quite a burden on users who are in environments that require auditing all unsafe.

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