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

Add Signal::as_str() to get representation as static string #1138

Merged
merged 1 commit into from Oct 29, 2019
Merged

Add Signal::as_str() to get representation as static string #1138

merged 1 commit into from Oct 29, 2019

Conversation

MikailBag
Copy link
Contributor

Motivation

Currently string representation of signal can be obtained with AsRef impl.
But it has downside: returned string's lifetime is bound to lifetime of signal, so &str must be converted to String. This allocation is avoidable, because as_ref() only returns static strings. To fix this problem, my PR adds Signal::as_str() method, which returns static string.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

This looks good. Could you also please include a CHANGELOG entry, showing users how to switch from the old method to the new one?

src/sys/signal.rs Outdated Show resolved Hide resolved
@MikailBag
Copy link
Contributor Author

Do docs look OK now?

@asomers
Copy link
Member

asomers commented Oct 26, 2019

Yeah, looks good. Just wrap the comment to 80 columns and I'll merge.

@MikailBag
Copy link
Contributor Author

Done

@MikailBag
Copy link
Contributor Author

Note: I changed signature of as_str to take self by-value, because Signal is Copy

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

Perfect! Just squash your commits and I'll merge them. I can't squash your commits for you because Nix uses the bors merge bot.

@MikailBag
Copy link
Contributor Author

Squashed.

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Oct 29, 2019
1138: Add Signal::as_str() to get representation as static string r=asomers a=MikailBag

# Motivation
Currently string representation of signal can be obtained with AsRef<str> impl.
But it has downside: returned string's lifetime is bound to lifetime of signal, so &str must be converted to String. This allocation is avoidable, because as_ref() only returns static strings. To fix this problem, my PR adds Signal::as_str() method, which returns static string.

Co-authored-by: Mikail Bagishov <bagishov.mikail@yandex.ru>
@bors
Copy link
Contributor

bors bot commented Oct 29, 2019

Build succeeded

@bors bors bot merged commit bacb85e into nix-rust:master Oct 29, 2019
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