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

Simplify Options by removing generic type parameters #438

Merged
merged 8 commits into from
Feb 26, 2022
Merged

Conversation

mgeisler
Copy link
Owner

@mgeisler mgeisler commented Feb 20, 2022

This PR removes the three generic type parameters from the Options struct. Before we had

pub struct Options<
    'a,
    WrapAlgo = Box<dyn wrap_algorithms::WrapAlgorithm>,
    WordSep = Box<dyn word_separators::WordSeparator>,
    WordSplit = Box<dyn word_splitters::WordSplitter>,
> {
    // ...
}

and now we simply have

pub struct Options<'a> {
    // ...
}

which is much easier on the eyes. A consequence of this is that the function signatures of wrap, fill, etc are simplified as well. The old functionality is kept intact and there are Custom enum variants which accept a function pointer for quick and easy extensions (though the functions cannot borrow anything — please let me know if this is a concern).

The change here is almost performance neutral with only a 1-2% regression:

% cargo criterion fill_optimal_fit_ascii/4800
String lengths/fill_optimal_fit_ascii/4800
                        time:   [225.48 us 225.50 us 225.51 us]
                        change: [+1.1664% +1.2851% +1.4765%] (p = 0.00 < 0.05)
                        Performance has regressed.

Somewhat surprisingly, this PR is also nearly neutral with regards to the binary size with one configuration taking 4 KB more than before:

Configuration Binary Size Delta Before Delta After
quick-and-dirty implementation 297 KB — KB — KB
textwrap without default features 309 KB 12 KB 16 KB
textwrap with smawk 321 KB 24 KB 24 KB
textwrap with unicode-width 321 KB 24 KB 24 KB
textwrap with unicode-linebreak 399 KB 102 KB 102 KB

As soon as we enable the default features, the size difference disappears.

This constructor was a left over from back when `Options` had a single
`WordSplitter` generic type parameter. Now that we have several
generic type parameters, having this constructor make one parameter
different from the others (for no good reason).

The `with_word_splitter` method was const, so we now lose the
ability to construct `Options` with a non-default `WordSplitter` in a
const context. If this is important to you, please get in contact so
we can look at making the builder methods const instead.
This test helped me confirm that the many traits worked the right way,
e.g., that dynamic dispatch was available when needed.

However, I will be removing the traits in the next few commits and
thus we don’t need the test any longer.
We don’t need this test with the simpler types: the types tell us
directly that cloning works.
@mgeisler mgeisler changed the title Simplify Options by removing traits Simplify Options by removing generic type parameters Feb 20, 2022
@mgeisler mgeisler force-pushed the remove-traits branch 2 times, most recently from db2b259 to feb3171 Compare February 20, 2022 19:20
This replaces the `WordSplitter` trait with an enum. The result is a
simplified `Options` type: it has one less generic type parameter.
@mgeisler mgeisler force-pushed the remove-traits branch 2 times, most recently from 6e926c4 to 1f6b65f Compare February 26, 2022 09:11
This replaces the `WordSeparator` trait with an enum. The result is a
simplified `Options` type: it has one less generic type parameter.
This replaces the `WordAlgorithm` trait with an enum. The result is a
simplified `Options` type: it has one less generic type parameter.
There are no trait objects now, only concrete types.
@mgeisler mgeisler force-pushed the remove-traits branch 2 times, most recently from 2d62107 to 2d2b02e Compare February 26, 2022 10:15
Before, the `OptimalFit` struct was implementing the old
`WrapAlgorithm` trait. It therefore made sense to say that the
struct (with its penalties) _were_ the wrapping algorithm. However,
now that we have a `WrapAlgorithm::OptimalFit` enum variant, the
struct should be renamed to reflect its purpose.
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.

1 participant