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

Assume conversions to byte slices are always successful #3

Merged
merged 3 commits into from
May 6, 2019

Conversation

koute
Copy link
Contributor

@koute koute commented May 4, 2019

AFAIK there isn't much point in returning a Result when converting from &[T] to &[u8] as those conversions should always be successful.

@sdroege
Copy link
Owner

sdroege commented May 6, 2019

This breaks the API, but that's fine. Thanks a lot!

Can you bump the version to 0.3.0 in Cargo.toml in a second commit? Then I'll merge it

@koute
Copy link
Contributor Author

koute commented May 6, 2019

Done! I've also updated the changelog.

Please don't release this on crates.io yet though. I have one more potential breaking change I'd like to create a PR for; I just wanted to wait for this one to get merged. (:

@sdroege
Copy link
Owner

sdroege commented May 6, 2019

Done! I've also updated the changelog.

Great, thanks :)

Please don't release this on crates.io yet though. I have one more potential breaking change I'd like to create a PR for; I just wanted to wait for this one to get merged. (:

Sure, what other change is that?

@sdroege sdroege merged commit db4dc0c into sdroege:master May 6, 2019
@koute
Copy link
Contributor Author

koute commented May 6, 2019

Sure, what other change is that?

Extending the Error type so that the error message is more descriptive, e.g. in case of an alignment mismatch the error message would be something like this: (I don't mind bikeshedding the exact message if you'd prefer something different)

cannot cast a &[u8] into a &[u16]: the slice's address is not divisible by the minimum alignment of u16 (= 2)

instead of the current "Wrong Alignment".

@sdroege
Copy link
Owner

sdroege commented May 6, 2019

Yeah that makes sense. Please put all the logic into the Display impl though and keep the Error type itself allocation-less :)

@koute
Copy link
Contributor Author

koute commented May 6, 2019

Yep. That's the idea. (:

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