Skip to content

Fix UB and a lot of small emprovements #1

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Fix UB and a lot of small emprovements #1

wants to merge 10 commits into from

Conversation

WaffleLapkin
Copy link

  • fix UB
  • move to edition 2018
  • add no_std support
  • remove unsafe code
  • impl Display for {ByteStr,ByteString}
  • adjust docs
  • clippy

@FraGag
Copy link
Owner

FraGag commented Nov 22, 2020

Thanks for the pull request!

I'm not sure about the addition of the Display impls. Per the documentation, "[...] Display is for user-facing output [...]", and escape sequences (\xNN) are not what I would consider "user-facing". That's why I omitted the Display impls in the first place. However, for strings that are "known" to consist only of printable ASCII characters (in the range \x20 to '\x7E`), then I can see that this impl would be nice to have. Do you have such a use case, or do you have another reason for adding these impls?

If we do add the Display impls, then I would appreciate it if you could add tests for them. You may just copy the debug_* tests, name them display_* and switch them to testing the Display impl.

@WaffleLapkin
Copy link
Author

The "user" may be a user of some programming tool, so I think that it's reasonable to provide Display impl.

Do you have such a use case

I've found your crate while discussing weird output of another crate (or app? I forgot),
so while I don't have such a use case, others probably do.

I'll add tests.

@WaffleLapkin
Copy link
Author

@FraGag would you be able to review/merge/release this? Your crate seems to be still used (around 6k monthly downloads), so it would be nice to fix UB in it.

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