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

Delegate to Chars and CharIndices #4

Merged
merged 1 commit into from
May 21, 2018
Merged

Conversation

chancancode
Copy link
Contributor

Closes #3

I did end up making a delegate create, so I figured I would put this together quickly to see how it feels in real code. Unfortunately, as @doomrobo pointed out in #2, there isn't a good way to implement clone since the state/pointers are not managed by us anymore.

src/lib.rs Outdated
// First, move the string
let mut owned = $struct {
s: s,
i: unsafe { uninitialized() }
Copy link
Owner

Choose a reason for hiding this comment

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

My personal "unsafe code policy" is that when leaving an unsafe block, the invariants that we are screwing with should be restored. So could you just merge these two blocks so that lines 52-60 are under one unsafe?

src/lib.rs Outdated
// Then, we can call .chars, which with have the same lifetime
// of the owner. We need the transmute to "widen" the lifetime
// into 'static which would allow us to store it in the owner.
owned.i = unsafe { transmute(owned.s.$method()) };
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to write it as transmute::<Chars, Chars<'static>(...) just to be explicit that it's only changing the lifetime, but that'd require an extra parameter to the macro...

@durka
Copy link
Owner

durka commented May 19, 2018

Looks great! Happy to be an early adopter of your macro :)

I meant to respond to your original issue, sorry for dropping that ball. If you can make that small change to the unsafe code I'll merge!

@chancancode
Copy link
Contributor Author

Fixed!

@durka durka merged commit 7433284 into durka:master May 21, 2018
@durka
Copy link
Owner

durka commented May 21, 2018

Awesome. I'll publish a new version soon, I guess it ought to be a minor version due to the as_str behavior fix. Since this is a fairly significant overhaul (given that it's a tiny crate!), would you like to be listed as one of the crate authors?

@chancancode chancancode deleted the delegate branch May 21, 2018 01:52
@chancancode
Copy link
Contributor Author

Don’t worry about it! We should probably work on RFC-ing this into stdlib though 😄

@durka
Copy link
Owner

durka commented May 21, 2018 via email

@durka
Copy link
Owner

durka commented Mar 15, 2020

Hello from the future... I was a Rust novice at the time, and didn't realize until two years later when nightly started complaining about mem::uninitialized that this trivially allows UB in safe code -- see #7.

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.

Consider delegating to Chars/CharIndices instead of re-implementing logic
2 participants