diff --git a/docs/testing.md b/docs/testing.md index 949a783..57ca213 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -356,7 +356,16 @@ so the next unimplemented method gets listed rather than silently failing — an a case that fails for any *other* reason is still a failure. Every `-m` knob is implemented: `-mm`, `-mc`, `-md`, `-ms`, `-mt` and `-ma`. -`-lc-`/`-ld-` are still not accepted at all. +`-lc-`/`-ld-` are accepted and mean no limit, which this line denied until it +was measured; README's "What the archiver cannot do yet" had already been +corrected in #129/#130 and the two sections disagreed. + +The figure they take is `parseMemWithPercents`'s, not the `-m` grammar's: a +bare number is MEGABYTES (`Utils.hs:71` — `"24" означает 24mb`). Reading it +with `method::parse_mem` instead made `-lc24` mean 16 MB, refused `-lc512` +outright and let `-lc8` reach PPMd as 256 bytes, where the model panicked. +Gated in `memlimit.rs`'s `a_bare_figure_is_megabytes` and +`a_bare_figure_is_not_a_power_of_two`. `-ma` is the one worth reading the harness for. It selects between the TWO paths of `splitFileTypes`, and the port had implemented neither clause of diff --git a/rust/darc-arc/src/memlimit.rs b/rust/darc-arc/src/memlimit.rs index 5a52c7d..42946e5 100644 --- a/rust/darc-arc/src/memlimit.rs +++ b/rust/darc-arc/src/memlimit.rs @@ -806,17 +806,50 @@ fn physical_memory_raw() -> u64 { /// /// The default unit is megabytes, `b` means bytes, and `%` or `p` means a /// percentage of `memory`. Anything else is an error there, and `None` here. +/// +/// The unit table is `parseNumber`'s (`Utils.hs:55`), which APPENDS the default +/// specifier to the string before reading it, so a bare `"512"` is read as +/// `"512m"`. `Utils.hs:71` states it in one line: `"24" означает 24mb`. +/// +/// That is NOT `method::parse_mem`'s table, which this used to delegate to. +/// There a bare figure is a POWER OF TWO, because the `-m` grammar spells a +/// 4 MB dictionary `-md22`. The two tables agree on `b`/`k`/`m`/`g` and +/// disagree on exactly the spelling `-lc` documents — README's option table +/// reads `-lc N` as "N MB" — so routing the memory limits through the method +/// table produced three wrong answers at once: +/// +/// * `-lc24` meant 16 MB (`1 << 24`), not 24 MB, and said nothing about it. +/// * `-lc512` was refused outright with "not a memory size", because +/// `1 << 512` does not fit and the method table returns `None` at 32. +/// * `-lc8` became 256 BYTES, which no PPMd model can be built in: it left the +/// error path entirely and panicked in `ppmd/model.rs` on a slice range of +/// 258 into 256. +/// +/// `t` and `^` are here because `parseNumber` has them; `^` is the explicit +/// power-of-two spelling, which is how a caller asks for what the method table +/// gave by accident. The suffix is lower-cased because `parseNumber` reads +/// `strLower`. pub fn parse_mem_with_percents(memory: u64, s: &str) -> Option { let digits: String = s.chars().take_while(char::is_ascii_digit).collect(); if digits.is_empty() { return None; } let n: u64 = digits.parse().ok()?; - match s[digits.len()..].chars().next() { + match s[digits.len()..] + .chars() + .next() + .map(|c| c.to_ascii_lowercase()) + { Some('%') | Some('p') => Some(memory.saturating_mul(n) / 100), - // Everything else is a plain size, and parse_mem owns the unit table - // so the two spellings cannot drift apart. - _ => crate::method::parse_mem(s).map(u64::from), + Some('b') => Some(n), + Some('k') => Some(n.saturating_mul(KB)), + // The default specifier, and the explicit spelling of it. + None | Some('m') => Some(n.saturating_mul(MB)), + Some('g') => Some(n.saturating_mul(1024 * MB)), + Some('t') => Some(n.saturating_mul(1024 * 1024 * MB)), + // `2 ^ readI digits`, in bytes. 64 and up cannot be represented. + Some('^') => (n < 64).then(|| 1u64 << n), + Some(_) => None, } } @@ -1333,5 +1366,68 @@ mod lzma2_and_lc_tests { assert_eq!(get_dictionary(&r), 8 * MB as u32); } + /// `Utils.hs:71`, verbatim: `"24" означает 24mb`. The figure `-lc` and + /// `-ld` are documented in is megabytes (README's option table: "Limit + /// memory for compression to N MB"), and a bare number is that figure. + #[test] + fn a_bare_figure_is_megabytes() { + assert_eq!(parse_mem_with_percents(0, "24"), Some(24 * MB)); + assert_eq!(parse_mem_with_percents(0, "512"), Some(512 * MB)); + assert_eq!(parse_mem_with_percents(0, "8"), Some(8 * MB)); + } + + /// The method table would have answered `1 << n` to all three above, so + /// these are the values that used to come back, pinned to keep the two + /// tables from being merged again by someone reading only the call site. + #[test] + fn a_bare_figure_is_not_a_power_of_two() { + assert_ne!(parse_mem_with_percents(0, "24"), Some(1 << 24)); + assert_ne!(parse_mem_with_percents(0, "8"), Some(1 << 8)); + // `1 << 512` has no answer at all, which is how `-lc512` came to be + // refused as "not a memory size" while being exactly what the option + // documents. + assert!(parse_mem_with_percents(0, "512").is_some()); + } + + /// `parseNumber` (`Utils.hs:55`) — the whole table, including the `^` + /// spelling a caller needs now that a bare figure no longer gives it. + #[test] + fn the_unit_table_is_parse_numbers() { + assert_eq!(parse_mem_with_percents(0, "512b"), Some(512)); + assert_eq!(parse_mem_with_percents(0, "32k"), Some(32 * KB)); + assert_eq!(parse_mem_with_percents(0, "8m"), Some(8 * MB)); + assert_eq!(parse_mem_with_percents(0, "2g"), Some(2 * 1024 * MB)); + assert_eq!(parse_mem_with_percents(0, "1t"), Some(1024 * 1024 * MB)); + assert_eq!(parse_mem_with_percents(0, "8^"), Some(256)); + // `strLower` runs before the table is consulted. + assert_eq!(parse_mem_with_percents(0, "8M"), Some(8 * MB)); + assert_eq!(parse_mem_with_percents(0, "512B"), Some(512)); + } + + /// Only the first suffix character is read, as `span isDigit` leaves it. + #[test] + fn only_the_first_suffix_character_is_read() { + assert_eq!(parse_mem_with_percents(0, "16kb"), Some(16 * KB)); + assert_eq!(parse_mem_with_percents(0, "1mb"), Some(MB)); + } + + /// Percentages are of the figure passed in, and keep working. + #[test] + fn percentages_are_of_the_memory_given() { + assert_eq!(parse_mem_with_percents(1000, "75%"), Some(750)); + assert_eq!(parse_mem_with_percents(1000, "75p"), Some(750)); + } + + /// Anything the table does not name is an error there and `None` here, + /// which is what makes the CLI print "not a memory size" for it. + #[test] + fn an_unknown_suffix_is_still_rejected() { + assert_eq!(parse_mem_with_percents(0, "8x"), None); + assert_eq!(parse_mem_with_percents(0, "abc"), None); + assert_eq!(parse_mem_with_percents(0, ""), None); + // 64 and up do not fit in the shift. + assert_eq!(parse_mem_with_percents(0, "64^"), None); + } + }