Allow wrap to borrow when preserve_trailing_space is true

[JesseCSlater]

This pull request modifies wrap_single_line_slow_path to borrow when preserve_trailing_space is set and the final word has no penalty. Previously, it was unnecessarily forcing a copy in this case. I believe that logic remains exactly the same in all other cases.

[JesseCSlater]

The correctness of this pull request relies on the invariant that Word.word and Word.whitespace are consecutive slices of the same string. It seems that this invariant is already expected by wrap_single_line_slow_path but I don't think it is documented anywhere and a custom word separator could theoretically break it.

[JesseCSlater]

Apologies, I made a silly mistake. I didn't realize that additional commits on my fork would be automatically included in this pull request. The extra two commits making wrap_single_line public were not intended to be included.

[mgeisler]

The correctness of this pull request relies on the invariant that Word.word and Word.whitespace are consecutive slices of the same string. It seems that this invariant is already expected by wrap_single_line_slow_path but I don't think it is documented anywhere and a custom word separator could theoretically break it.

Yes, you are correct! The logic in wrap_single_line_slow_path assumes this. I also think you're correct that this is not documented anywhere.

I kind of consider wrap a convenience function: I have always hoped that people with more advanced requirements would use the lower-level primitives directly. That is, if someone has a case where the fragments don't correspond, then they should still be able to use the wrap algorithm to get correct lines, but they'll have to put them together themselves.

I don't know if anyone has ever done this, but it was the idea behind the design here. The name textwrap::core has always bothered me a little: perhaps I should have called the module textwrap::primitives instead?

The extra two commits making wrap_single_line public were not intended to be included.

Thanks for noticing and fixing that!

I looked at the change now, sorry about the wait! Would you be able to add a new unit test which demonstrates that we are now borrowing more than we use to do? It's frankly a bit subtle, so I would love to have a test to confirm what is going on.

[JesseCSlater]

Thanks for the reply! Just added a unit test which failed before the change.

I'll restate what I changed a little more clearly, because I agree it's subtle.

Previously, wrap_single_line_slow_path was unconditionally creating a Cow borrowing the line without trailing spaces and then appending the spaces if necessary. The append was unneccesarily forcing the line to become owned. This meant that if preserve_trailing_space was true, every line which ended with spaces (likely every line except the last) would become owned.

After my change, wrap_single_line_slow_path creates a Cow borrowing the line with the trailing spaces if preserve_trailing_space is set, preventing the append.

[mgeisler]

Previously, wrap_single_line_slow_path was unconditionally creating a Cow borrowing the line without trailing spaces and then appending the spaces if necessary. The append was unneccesarily forcing the line to become owned. This meant that if preserve_trailing_space was true, every line which ended with spaces (likely every line except the last) would become owned.

Thank you for this, now I see why this is a big deal! Cc @HidenoriKobayashi as well, who wrote the code for this in #587.