From 0c13bae6bfe05393857e626b0c86e2527c3bc077 Mon Sep 17 00:00:00 2001 From: Ideflop Date: Wed, 5 Apr 2023 21:25:49 +0200 Subject: [PATCH 1/2] factor: add -h/--exponents option --- src/uu/factor/src/cli.rs | 40 +++++++++++++++++++++++ src/uu/factor/src/factor.rs | 14 ++++++-- tests/by-util/test_factor.rs | 63 ++++++++++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 2 deletions(-) diff --git a/src/uu/factor/src/cli.rs b/src/uu/factor/src/cli.rs index 956b6aa9f0..104992c39b 100644 --- a/src/uu/factor/src/cli.rs +++ b/src/uu/factor/src/cli.rs @@ -27,9 +27,30 @@ const ABOUT: &str = help_about!("factor.md"); const USAGE: &str = help_usage!("factor.md"); mod options { + pub static EXPONENTS: &str = "exponents"; + pub static HELP: &str = "help"; pub static NUMBER: &str = "NUMBER"; } +mod exponent_options { + + pub static mut PRINT_EXPONENTS: bool = false; + + // Safety + // + // This function contains unsafe code that modifies the static mutable variable + // PRINT_EXPONENTS. + // This function is called ones inside uumain if the -h or --exponents flag + // is found. + // If print_exponents() is called and PRINT_EXPONENTS is true then the p^e + // output format will be used. + pub fn print_exponents() { + unsafe { + PRINT_EXPONENTS = true; + } + } +} + fn print_factors_str( num_str: &str, w: &mut io::BufWriter, @@ -50,6 +71,11 @@ fn print_factors_str( #[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { let matches = uu_app().try_get_matches_from(args)?; + + if matches.get_flag(options::EXPONENTS) { + exponent_options::print_exponents(); + }; + let stdout = stdout(); // We use a smaller buffer here to pass a gnu test. 4KiB appears to be the default pipe size for bash. let mut w = io::BufWriter::with_capacity(4 * 1024, stdout.lock()); @@ -86,5 +112,19 @@ pub fn uu_app() -> Command { .about(ABOUT) .override_usage(format_usage(USAGE)) .infer_long_args(true) + .disable_help_flag(true) .arg(Arg::new(options::NUMBER).action(ArgAction::Append)) + .arg( + Arg::new(options::EXPONENTS) + .short('h') + .long(options::EXPONENTS) + .help("Print factors in the form p^e") + .action(ArgAction::SetTrue), + ) + .arg( + Arg::new(options::HELP) + .long(options::HELP) + .help("Print help information.") + .action(ArgAction::Help), + ) } diff --git a/src/uu/factor/src/factor.rs b/src/uu/factor/src/factor.rs index 5d16153423..68b8c1be19 100644 --- a/src/uu/factor/src/factor.rs +++ b/src/uu/factor/src/factor.rs @@ -9,6 +9,7 @@ use smallvec::SmallVec; use std::cell::RefCell; use std::fmt; +use crate::exponent_options::PRINT_EXPONENTS; use crate::numeric::{Arithmetic, Montgomery}; use crate::{miller_rabin, rho, table}; @@ -98,8 +99,17 @@ impl fmt::Display for Factors { v.sort_unstable(); for (p, exp) in v.iter() { - for _ in 0..*exp { - write!(f, " {p}")?; + // Safety + // + // Using PRINT_EXPONENTS here is safe because it is only ever set to + // true in the uumain function, and the uumain function is the only + // function that calls this function. + if unsafe { PRINT_EXPONENTS } && *exp > 1 { + write!(f, " {p}^{exp}")?; + } else { + for _ in 0..*exp { + write!(f, " {p}")?; + } } } diff --git a/tests/by-util/test_factor.rs b/tests/by-util/test_factor.rs index 23da2ea69f..74864a7922 100644 --- a/tests/by-util/test_factor.rs +++ b/tests/by-util/test_factor.rs @@ -30,6 +30,12 @@ fn test_invalid_arg() { new_ucmd!().arg("--definitely-invalid").fails().code_is(1); } +#[test] +fn test_valid_arg_exponents() { + new_ucmd!().arg("-h").succeeds().code_is(0); + new_ucmd!().arg("--exponents").succeeds().code_is(0); +} + #[test] #[cfg(feature = "sort")] fn test_parallel() { @@ -103,6 +109,34 @@ fn test_first_1000_integers() { ); } +#[test] +fn test_first_1000_integers_with_exponents() { + extern crate sha1; + use hex_literal::hex; + use sha1::{Digest, Sha1}; + + let n_integers = 1000; + let mut input_string = String::new(); + for i in 0..=n_integers { + input_string.push_str(&(format!("{i} "))[..]); + } + + println!("STDIN='{input_string}'"); + let result = new_ucmd!() + .arg("-h") + .pipe_in(input_string.as_bytes()) + .succeeds(); + + // Using factor from GNU Coreutils 9.2 + // `seq 0 1000 | factor | sha1sum` => "45f5f758a9319870770bd1fec2de23d54331944d" + let mut hasher = Sha1::new(); + hasher.update(result.stdout()); + let hash_check = hasher.finalize(); + assert_eq!( + hash_check[..], + hex!("45f5f758a9319870770bd1fec2de23d54331944d") + ); +} #[test] fn test_cli_args() { // Make sure that factor works with CLI arguments as well. @@ -282,6 +316,35 @@ fn run(input_string: &[u8], output_string: &[u8]) { .stdout_is(String::from_utf8(output_string.to_owned()).unwrap()); } +#[test] +fn test_primes_with_exponents() { + let mut input_string = String::new(); + let mut output_string = String::new(); + for primes in PRIMES_BY_BITS.iter() { + for &prime in *primes { + input_string.push_str(&(format!("{prime} "))[..]); + output_string.push_str(&(format!("{prime}: {prime}\n"))[..]); + } + } + + println!( + "STDIN='{}'", + String::from_utf8_lossy(input_string.as_bytes()) + ); + println!( + "STDOUT(expected)='{}'", + String::from_utf8_lossy(output_string.as_bytes()) + ); + + // run factor with --exponents + new_ucmd!() + .timeout(Duration::from_secs(240)) + .arg("--exponents") + .pipe_in(input_string) + .run() + .stdout_is(String::from_utf8(output_string.as_bytes().to_owned()).unwrap()); +} + const PRIMES_BY_BITS: &[&[u64]] = &[ PRIMES14, PRIMES15, PRIMES16, PRIMES17, PRIMES18, PRIMES19, PRIMES20, PRIMES21, PRIMES22, PRIMES23, PRIMES24, PRIMES25, PRIMES26, PRIMES27, PRIMES28, PRIMES29, PRIMES30, PRIMES31, From f4e2a19033694346d412294df43d1680758d1bc6 Mon Sep 17 00:00:00 2001 From: Ideflop Date: Fri, 14 Apr 2023 10:51:35 +0200 Subject: [PATCH 2/2] Factor: Rebase, add -h/--exponents options --- Cargo.lock | 4 +-- Cargo.toml | 2 +- src/uu/factor/factor.md | 2 +- src/uu/factor/src/cli.rs | 40 ++++++++++----------------- src/uu/factor/src/factor.rs | 9 ++---- tests/benches/factor/Cargo.toml | 4 ++- tests/benches/factor/benches/gcd.rs | 4 +-- tests/benches/factor/benches/table.rs | 33 +++++++++------------- tests/by-util/test_factor.rs | 16 ++++------- 9 files changed, 45 insertions(+), 69 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 54a080c9c0..0fbba8f987 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1125,9 +1125,9 @@ checksum = "7f24254aa9a54b5c858eaee2f5bccdb46aaf0e486a595ed5fd8f86ba55232a70" [[package]] name = "hex-literal" -version = "0.4.0" +version = "0.4.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "4bcb5b3e439c92a7191df2f9bbe733de8de55c3f86368cdb1c63f8be7e9e328e" +checksum = "6fe2267d4ed49bc07b63801559be28c718ea06c4738b7a03c94df7386d2cde46" [[package]] name = "hostname" diff --git a/Cargo.toml b/Cargo.toml index 980860749f..5b6b71edf9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -487,7 +487,7 @@ unindent = "0.2" uucore = { workspace=true, features=["entries", "process", "signals"] } walkdir = { workspace=true } is-terminal = { workspace=true } -hex-literal = "0.4.0" +hex-literal = "0.4.1" rstest = "0.17.0" [target.'cfg(any(target_os = "linux", target_os = "android"))'.dev-dependencies] diff --git a/src/uu/factor/factor.md b/src/uu/factor/factor.md index b5bec6039b..ccce4f2529 100644 --- a/src/uu/factor/factor.md +++ b/src/uu/factor/factor.md @@ -1,7 +1,7 @@ # factor ``` -factor [NUMBER]... +factor [OPTION]... [NUMBER]... ``` Print the prime factors of the given NUMBER(s). diff --git a/src/uu/factor/src/cli.rs b/src/uu/factor/src/cli.rs index 104992c39b..9f3d55db9f 100644 --- a/src/uu/factor/src/cli.rs +++ b/src/uu/factor/src/cli.rs @@ -32,29 +32,11 @@ mod options { pub static NUMBER: &str = "NUMBER"; } -mod exponent_options { - - pub static mut PRINT_EXPONENTS: bool = false; - - // Safety - // - // This function contains unsafe code that modifies the static mutable variable - // PRINT_EXPONENTS. - // This function is called ones inside uumain if the -h or --exponents flag - // is found. - // If print_exponents() is called and PRINT_EXPONENTS is true then the p^e - // output format will be used. - pub fn print_exponents() { - unsafe { - PRINT_EXPONENTS = true; - } - } -} - fn print_factors_str( num_str: &str, w: &mut io::BufWriter, factors_buffer: &mut String, + print_exponents: bool, ) -> Result<(), Box> { num_str .trim() @@ -62,7 +44,13 @@ fn print_factors_str( .map_err(|e| e.into()) .and_then(|x| { factors_buffer.clear(); - writeln!(factors_buffer, "{}:{}", x, factor(x))?; + // If print_exponents is true, use the alternate format specifier {:#} from fmt to print the factors + // of x in the form of p^e. + if print_exponents { + writeln!(factors_buffer, "{}:{:#}", x, factor(x))?; + } else { + writeln!(factors_buffer, "{}:{}", x, factor(x))?; + } w.write_all(factors_buffer.as_bytes())?; Ok(()) }) @@ -72,9 +60,8 @@ fn print_factors_str( pub fn uumain(args: impl uucore::Args) -> UResult<()> { let matches = uu_app().try_get_matches_from(args)?; - if matches.get_flag(options::EXPONENTS) { - exponent_options::print_exponents(); - }; + // If matches find --exponents flag than variable print_exponents is true and p^e output format will be used. + let print_exponents = matches.get_flag(options::EXPONENTS); let stdout = stdout(); // We use a smaller buffer here to pass a gnu test. 4KiB appears to be the default pipe size for bash. @@ -83,7 +70,8 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { if let Some(values) = matches.get_many::(options::NUMBER) { for number in values { - if let Err(e) = print_factors_str(number, &mut w, &mut factors_buffer) { + if let Err(e) = print_factors_str(number, &mut w, &mut factors_buffer, print_exponents) + { show_warning!("{}: {}", number.maybe_quote(), e); } } @@ -92,7 +80,9 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> { let lines = stdin.lock().lines(); for line in lines { for number in line.unwrap().split_whitespace() { - if let Err(e) = print_factors_str(number, &mut w, &mut factors_buffer) { + if let Err(e) = + print_factors_str(number, &mut w, &mut factors_buffer, print_exponents) + { show_warning!("{}: {}", number.maybe_quote(), e); } } diff --git a/src/uu/factor/src/factor.rs b/src/uu/factor/src/factor.rs index 68b8c1be19..a87f4219e7 100644 --- a/src/uu/factor/src/factor.rs +++ b/src/uu/factor/src/factor.rs @@ -9,7 +9,6 @@ use smallvec::SmallVec; use std::cell::RefCell; use std::fmt; -use crate::exponent_options::PRINT_EXPONENTS; use crate::numeric::{Arithmetic, Montgomery}; use crate::{miller_rabin, rho, table}; @@ -98,13 +97,9 @@ impl fmt::Display for Factors { let v = &mut (self.0).borrow_mut().0; v.sort_unstable(); + let include_exponents = f.alternate(); for (p, exp) in v.iter() { - // Safety - // - // Using PRINT_EXPONENTS here is safe because it is only ever set to - // true in the uumain function, and the uumain function is the only - // function that calls this function. - if unsafe { PRINT_EXPONENTS } && *exp > 1 { + if include_exponents && *exp > 1 { write!(f, " {p}^{exp}")?; } else { for _ in 0..*exp { diff --git a/tests/benches/factor/Cargo.toml b/tests/benches/factor/Cargo.toml index efada4b00e..26900e78a9 100644 --- a/tests/benches/factor/Cargo.toml +++ b/tests/benches/factor/Cargo.toml @@ -7,6 +7,8 @@ description = "Benchmarks for the uu_factor integer factorization tool" homepage = "https://github.com/uutils/coreutils" edition = "2021" +[workspace] + [dependencies] uu_factor = { path = "../../../src/uu/factor" } @@ -14,7 +16,7 @@ uu_factor = { path = "../../../src/uu/factor" } array-init = "2.0.0" criterion = "0.3" rand = "0.8" -rand_chacha = "0.2.2" +rand_chacha = "0.3.1" [[bench]] name = "gcd" diff --git a/tests/benches/factor/benches/gcd.rs b/tests/benches/factor/benches/gcd.rs index f2bae51c74..a9d2a8c55e 100644 --- a/tests/benches/factor/benches/gcd.rs +++ b/tests/benches/factor/benches/gcd.rs @@ -6,7 +6,7 @@ fn gcd(c: &mut Criterion) { // Deterministic RNG; use an explicitly-named RNG to guarantee stability use rand::{RngCore, SeedableRng}; use rand_chacha::ChaCha8Rng; - const SEED: u64 = 0xa_b4d_1dea_dead_cafe; + const SEED: u64 = 0xab4d_1dea_dead_cafe; let mut rng = ChaCha8Rng::seed_from_u64(SEED); std::iter::repeat_with(move || (rng.next_u64(), rng.next_u64())) @@ -22,7 +22,7 @@ fn gcd(c: &mut Criterion) { }, ); } - group.finish() + group.finish(); } criterion_group!(benches, gcd); diff --git a/tests/benches/factor/benches/table.rs b/tests/benches/factor/benches/table.rs index 7f749a10ff..59e8db1f3b 100644 --- a/tests/benches/factor/benches/table.rs +++ b/tests/benches/factor/benches/table.rs @@ -7,12 +7,7 @@ fn table(c: &mut Criterion) { check_personality(); const INPUT_SIZE: usize = 128; - assert!( - INPUT_SIZE % CHUNK_SIZE == 0, - "INPUT_SIZE ({}) is not divisible by CHUNK_SIZE ({})", - INPUT_SIZE, - CHUNK_SIZE - ); + let inputs = { // Deterministic RNG; use an explicitly-named RNG to guarantee stability use rand::{RngCore, SeedableRng}; @@ -29,42 +24,40 @@ fn table(c: &mut Criterion) { let a_str = format!("{:?}", a); group.bench_with_input(BenchmarkId::new("factor_chunk", &a_str), &a, |b, &a| { b.iter(|| { - let mut n_s = a.clone(); + let mut n_s = a; let mut f_s: [_; INPUT_SIZE] = array_init(|_| Factors::one()); for (n_s, f_s) in n_s.chunks_mut(CHUNK_SIZE).zip(f_s.chunks_mut(CHUNK_SIZE)) { - factor_chunk(n_s.try_into().unwrap(), f_s.try_into().unwrap()) + factor_chunk(n_s.try_into().unwrap(), f_s.try_into().unwrap()); } - }) + }); }); group.bench_with_input(BenchmarkId::new("factor", &a_str), &a, |b, &a| { b.iter(|| { - let mut n_s = a.clone(); + let mut n_s = a; let mut f_s: [_; INPUT_SIZE] = array_init(|_| Factors::one()); for (n, f) in n_s.iter_mut().zip(f_s.iter_mut()) { - factor(n, f) + factor(n, f); } - }) + }); }); } - group.finish() + group.finish(); } #[cfg(target_os = "linux")] fn check_personality() { use std::fs; const ADDR_NO_RANDOMIZE: u64 = 0x0040000; - const PERSONALITY_PATH: &'static str = "/proc/self/personality"; + const PERSONALITY_PATH: &str = "/proc/self/personality"; let p_string = fs::read_to_string(PERSONALITY_PATH) - .expect(&format!("Couldn't read '{}'", PERSONALITY_PATH)) - .strip_suffix("\n") + .unwrap_or_else(|_| panic!("Couldn't read '{}'", PERSONALITY_PATH)) + .strip_suffix('\n') .unwrap() .to_owned(); - let personality = u64::from_str_radix(&p_string, 16).expect(&format!( - "Expected a hex value for personality, got '{:?}'", - p_string - )); + let personality = u64::from_str_radix(&p_string, 16) + .unwrap_or_else(|_| panic!("Expected a hex value for personality, got '{:?}'", p_string)); if personality & ADDR_NO_RANDOMIZE == 0 { eprintln!( "WARNING: Benchmarking with ASLR enabled (personality is {:x}), results might not be reproducible.", diff --git a/tests/by-util/test_factor.rs b/tests/by-util/test_factor.rs index 74864a7922..59940e2e3a 100644 --- a/tests/by-util/test_factor.rs +++ b/tests/by-util/test_factor.rs @@ -8,19 +8,16 @@ // spell-checker:ignore (methods) hexdigest -use crate::common::util::{AtPath, TestScenario}; +use crate::common::util::TestScenario; use std::time::{Duration, SystemTime}; #[path = "../../src/uu/factor/sieve.rs"] mod sieve; -extern crate conv; -extern crate rand; - -use self::rand::distributions::{Distribution, Uniform}; -use self::rand::{rngs::SmallRng, Rng, SeedableRng}; use self::sieve::Sieve; +use rand::distributions::{Distribution, Uniform}; +use rand::{rngs::SmallRng, Rng, SeedableRng}; const NUM_PRIMES: usize = 10000; const NUM_TESTS: usize = 100; @@ -39,6 +36,7 @@ fn test_valid_arg_exponents() { #[test] #[cfg(feature = "sort")] fn test_parallel() { + use crate::common::util::AtPath; use hex_literal::hex; use sha1::{Digest, Sha1}; use std::{fs::OpenOptions, time::Duration}; @@ -86,7 +84,6 @@ fn test_parallel() { #[test] fn test_first_1000_integers() { - extern crate sha1; use hex_literal::hex; use sha1::{Digest, Sha1}; @@ -111,7 +108,6 @@ fn test_first_1000_integers() { #[test] fn test_first_1000_integers_with_exponents() { - extern crate sha1; use hex_literal::hex; use sha1::{Digest, Sha1}; @@ -128,7 +124,7 @@ fn test_first_1000_integers_with_exponents() { .succeeds(); // Using factor from GNU Coreutils 9.2 - // `seq 0 1000 | factor | sha1sum` => "45f5f758a9319870770bd1fec2de23d54331944d" + // `seq 0 1000 | factor -h | sha1sum` => "45f5f758a9319870770bd1fec2de23d54331944d" let mut hasher = Sha1::new(); hasher.update(result.stdout()); let hash_check = hasher.finalize(); @@ -151,7 +147,7 @@ fn test_cli_args() { #[test] fn test_random() { - use conv::prelude::*; + use conv::prelude::ValueFrom; let log_num_primes = f64::value_from(NUM_PRIMES).unwrap().log2().ceil(); let primes = Sieve::primes().take(NUM_PRIMES).collect::>();