Skip to content

Commit

Permalink
Change penalties to non-negative numbers
Browse files Browse the repository at this point in the history
The optimization problem solved by the optimal-fit algorithm is
fundamentally a minimization problem. It is therefore not sensible to
allow negative penalties since all penalties are there to discourage
certain features:

* `nline_penalty` discourages breaks with more lines than necessary,

* `overflow_penalty` discourages lines longer than the line width,

* `short_last_line_penalty` discourages short last lines,

* `hyphen_penalty` discourages hyphenation

Making this change surfaces the overflow bug behind #247 and #416.
This will be fixed next via #421 and this commit can be seen as a way
of simplifying that PR.
  • Loading branch information
mgeisler committed Jan 9, 2022
1 parent 89d4782 commit 4a1283a
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 21 deletions.
16 changes: 8 additions & 8 deletions examples/wasm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,22 +250,22 @@ pub enum WasmWrapAlgorithm {
#[wasm_bindgen]
#[derive(Copy, Clone, Debug, Default)]
pub struct WasmOptimalFit {
pub nline_penalty: i32,
pub overflow_penalty: i32,
pub nline_penalty: usize,
pub overflow_penalty: usize,
pub short_last_line_fraction: usize,
pub short_last_line_penalty: i32,
pub hyphen_penalty: i32,
pub short_last_line_penalty: usize,
pub hyphen_penalty: usize,
}

#[wasm_bindgen]
impl WasmOptimalFit {
#[wasm_bindgen(constructor)]
pub fn new(
nline_penalty: i32,
overflow_penalty: i32,
nline_penalty: usize,
overflow_penalty: usize,
short_last_line_fraction: usize,
short_last_line_penalty: i32,
hyphen_penalty: i32,
short_last_line_penalty: usize,
hyphen_penalty: usize,
) -> WasmOptimalFit {
WasmOptimalFit {
nline_penalty,
Expand Down
6 changes: 3 additions & 3 deletions examples/wasm/www/index.html
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ <h1>Textwrap WebAssembly Demo</h1>
<div class="option">
<label for="overflow-penalty">Overflow penalty:</label>
<input type="number" id="overflow-penalty-text">
<input type="range" id="overflow-penalty" min="-1000" max="10000" value="7500">
<input type="range" id="overflow-penalty" min="0" max="10000" value="7500">
</div>

<div class="option">
Expand All @@ -109,13 +109,13 @@ <h1>Textwrap WebAssembly Demo</h1>
<div class="option">
<label for="short-last-line-penalty">Short line penalty:</label>
<input type="number" id="short-last-line-penalty-text">
<input type="range" id="short-last-line-penalty" min="-2000" max="2000" value="100">
<input type="range" id="short-last-line-penalty" min="0" max="2000" value="100">
</div>

<div class="option">
<label for="hyphen-penalty">Hyphen penalty:</label>
<input type="number" id="hyphen-penalty-text">
<input type="range" id="hyphen-penalty" min="-2000" max="2000" value="100">
<input type="range" id="hyphen-penalty" min="0" max="2000" value="100">
</div>
</div>

Expand Down
8 changes: 4 additions & 4 deletions fuzz/fuzz_targets/wrap_optimal_fit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ use textwrap::wrap_algorithms::{wrap_optimal_fit, OptimalFit};

#[derive(Arbitrary, Debug)]
struct Penalties {
nline_penalty: i32,
overflow_penalty: i32,
nline_penalty: usize,
overflow_penalty: usize,
short_last_line_fraction: usize,
short_last_line_penalty: i32,
hyphen_penalty: i32,
short_last_line_penalty: usize,
hyphen_penalty: usize,
}

impl Into<OptimalFit> for Penalties {
Expand Down
11 changes: 11 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1392,7 +1392,18 @@ mod tests {
}

#[test]
#[cfg(not(feature = "smawk"))]
fn max_width() {
// No overflow for the first-fit wrap algorithm.
assert_eq!(wrap("foo bar", usize::max_value()), vec!["foo bar"]);
}

#[test]
#[cfg(feature = "smawk")]
#[should_panic(expected = "attempt to multiply with overflow")]
fn max_width() {
// The optimal-fit algorithm overflows for extreme line
// widths. See #247 and #416 for details..
assert_eq!(wrap("foo bar", usize::max_value()), vec!["foo bar"]);
}

Expand Down
12 changes: 6 additions & 6 deletions src/wrap_algorithms/optimal_fit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use crate::wrap_algorithms::WrapAlgorithm;
pub struct OptimalFit {
/// Per-line penalty. This is added for every line, which makes it
/// expensive to output more lines than the minimum required.
pub nline_penalty: i32,
pub nline_penalty: usize,

/// Per-character cost for lines that overflow the target line width.
///
Expand Down Expand Up @@ -67,7 +67,7 @@ pub struct OptimalFit {
/// character. If it overflows by more than one character, the
/// overflow penalty will quickly outgrow the cost of the gap, as
/// seen above.
pub overflow_penalty: i32,
pub overflow_penalty: usize,

/// When should the a single word on the last line be considered
/// "too short"?
Expand Down Expand Up @@ -128,10 +128,10 @@ pub struct OptimalFit {
/// Penalty for a last line with a single short word.
///
/// Set this to zero if you do not want to penalize short last lines.
pub short_last_line_penalty: i32,
pub short_last_line_penalty: usize,

/// Penalty for lines ending with a hyphen.
pub hyphen_penalty: i32,
pub hyphen_penalty: usize,
}

impl OptimalFit {
Expand Down Expand Up @@ -308,12 +308,12 @@ pub fn wrap_optimal_fit<'a, 'b, T: Fragment>(
// Next, we add a penalty depending on the line length.
if line_width > target_width {
// Lines that overflow get a hefty penalty.
let overflow = (line_width - target_width) as i32;
let overflow = line_width - target_width;
cost += overflow * penalties.overflow_penalty;
} else if j < fragments.len() {
// Other lines (except for the last line) get a milder
// penalty which depend on the size of the gap.
let gap = (target_width - line_width) as i32;
let gap = target_width - line_width;
cost += gap * gap;
} else if i + 1 == j && line_width < target_width / penalties.short_last_line_fraction {
// The last line can have any size gap, but we do add a
Expand Down

0 comments on commit 4a1283a

Please sign in to comment.