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

Remove copy from Display implementation #82

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

Conversation

ThePerkinrex
Copy link
Contributor

Removes possible allocation, removing to_string.

Maybe this is not allowed, as it is a possible breaking change, as every T: Display implements ToString but not the other way around.

I thought it may be positive to note that the old implementation allocated when not needed, like when LocatedSpan<&str> is used

Removes possible allocation, removing to_string
@progval
Copy link
Collaborator

progval commented Jan 8, 2022

Hmm, interesting. Sadly, rust-lang/rust#31844 isn't stable yet, so we can't have both; and adding a cfg feature seems overkill (and doesn't really solve the problem since both can't be used in the same application).

Do you know any type that implements ToString but not Display?

@progval progval changed the title Update Display implementation Remove copy from Display implementation Jan 8, 2022
@ThePerkinrex
Copy link
Contributor Author

Not in std, but anyone could create it, in a dependent crate, for example.

That would mean that if somebody did that, which isn't recommended, and I think clippy advises against it, but it is possible, they would have a breaking change when updating.

Anyways, this isn't much, as I think not many people would use display on this type directly, but it could be something to note, to add it to the next breaking release (v4.0.0)

I just wanted to let you know of this

@progval progval added this to the v5.0.0 milestone Jul 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants