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

Wordsplitter specialization #77

Merged
merged 3 commits into from
Aug 24, 2017
Merged

Wordsplitter specialization #77

merged 3 commits into from
Aug 24, 2017

Conversation

mgeisler
Copy link
Owner

This PR changes the Wrapper struct so it becomes generic over the type of word splitter used. I think this gives a somewhat better API, but it has the cost that one cannot change the word splitter used on an existing wrapper. I don't think that is something one would do very open, though.

@hcpl
Copy link
Contributor

hcpl commented Aug 24, 2017

Looking forward to this getting merged!

I already implemented Wrapper::wrap_iter() method but got stuck with wrap_iter() function because of transferring boxed trait objects to the iterator while making the iterator outlive a Wrapper instance. Since this PR removes boxing altogether, I can then proceed smoothly.

This removes the need to box the Wordsplitter, which in turn gives a
nicer API. It also makes it possible to have more optimized code,
though the benchmarks don't show any consistent improvement.
This function makes it easy to create a terminal-width sized Wrapper
while also using a custom WordSplitter.
@mgeisler
Copy link
Owner Author

@hcpl Hi! Oh, that's great to hear :-)

I'm still learning Rust, so any tips about how I can improve the API further will be very helpful...

@mgeisler mgeisler merged commit 77a26af into master Aug 24, 2017
@hcpl
Copy link
Contributor

hcpl commented Aug 24, 2017

I guess this is less about API being nice and more about practical reasons: since we can just create a new instance when we need another word splitter, we can eliminate redundant boxing. Which coincidentally enables some nifty features like cloning and allows the compiler to do more optimizations. And all that for free!

@mgeisler
Copy link
Owner Author

@hcpl Yeah, makes sense! I wasn't able to measure any real performance improvements, though. But being able to clone a Wrapper sounds like a nice feature :-)

You mentioned a wrap_iter method -- would that basically be #59?

@hcpl
Copy link
Contributor

hcpl commented Aug 24, 2017

Yes, and I'm adapting my current implementation to changes now

@mgeisler mgeisler deleted the wordsplitter-specialization branch December 17, 2017 23:02
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