Hi Padraig,
On 5/8/22 14:44, Pádraig Brady wrote:
> I'll apply the attached later.
I found some nits - see below.
> From f83b1e7b1c6d2a2c0211cc1097dc165a1918d8f3 Mon Sep 17 00:00:00 2001
> From: Rasmus Villemoes <rasmus.villem...@prevas.dk>
> Date: Wed, 27 Apr 2022 12:07:20 +0200
> Subject: [PATCH] factor: --exponents: new option for printing in x^y format
>
> When factoring numbers that have a large 2^n factor, it can be hard to
> eyeball just how many 2's there are. Add an option to print each prime
> power factor in the x^y format (omitting the exponent when it is 1).
>
> * src/factor.c: Add --exponents option for printing in x^y format.
also -h.
> * doc/coreutils.texi (factor invocation): Document the new option.
> * tests/factor/create-test.sh: Add logic for passing options to factor.
> * tests/factor/create-test.sh: Add test case for `factor -h`.
> * tests/factor/run.sh: Honour options passed from the test case.
> * tests/local.mk (factor_tests): Add tf37.sh.
> * THANKS.in: Add previous suggester
s/$/./
> diff --git a/doc/coreutils.texi b/doc/coreutils.texi
> index b1ec7c61c..586ae7b34 100644
> --- a/doc/coreutils.texi
> +++ b/doc/coreutils.texi
> @@ -18622,16 +18622,23 @@ These programs do numerically-related operations.
> @command{factor} prints prime factors. Synopses:
s/Synpses/Synopsis/ ... it's singular now.
> @example
> -factor [@var{number}]@dots{}
> -factor @var{option}
> +factor [@var{option}]@dots{} [@var{number}]@dots{}
> @end example
>
> If no @var{number} is specified on the command line, @command{factor} reads
> numbers from standard input, delimited by newlines, tabs, or spaces.
>
> -The @command{factor} command supports only a small number of options:
> +The @command{factor} command supports supports the following options:
Like in most other programs, we could now simplify and reference the Common
options.:
+ The program accepts the following options. Also see @ref{Common options}.
and remove @item --help and @item --version.
> @table @samp
> +@item -h
> +@itemx --exponents
> +@opindex -h
> +@opindex --exponents
> +print factors in the form @math{p^e}, rather than repeating
s/^print/Print/
Here it's p^e, in other places it is x^y. We should be consistent.
> +the prime @samp{p}, @samp{e} times. If the exponent @samp{e} is 1,
> +then it is omitted.
> +
A little example would be nice.
> @item --help
> Print a short help on standard output, then exit without further
> processing.
> diff --git a/src/factor.c b/src/factor.c
> index 66ce28b84..83eda47d9 100644
> --- a/src/factor.c
> +++ b/src/factor.c
> @@ -226,12 +226,16 @@ enum
>
> static struct option const long_options[] =
> {
> + {"exponents", no_argument, NULL, 'h'},
> {"-debug", no_argument, NULL, DEV_DEBUG_OPTION},
> {GETOPT_HELP_OPTION_DECL},
> {GETOPT_VERSION_OPTION_DECL},
> {NULL, 0, NULL, 0}
> };
>
> +/* If true, use p^e output format. */
> +static bool print_exponents;
> +
> struct factors
> {
> uintmax_t plarge[2]; /* Can have a single large factor */
> @@ -2457,6 +2461,12 @@ print_factors_single (uintmax_t t1, uintmax_t t0)
> {
> lbuf_putc (' ');
> print_uintmaxes (0, factors.p[j]);
> + if (print_exponents && factors.e[j] > 1)
> + {
> + lbuf_putc ('^');
> + lbuf_putint (factors.e[j], 0);
> + break;
> + }
> }
>
> if (factors.plarge[1])
> @@ -2525,6 +2535,11 @@ print_factors (char const *input)
> {
> putchar (' ');
> mpz_out_str (stdout, 10, factors.p[j]);
> + if (print_exponents && factors.e[j] > 1)
> + {
> + printf ("^%lu", factors.e[j]);
> + break;
> + }
> }
>
> mp_factor_clear (&factors);
> @@ -2542,15 +2557,18 @@ usage (int status)
> else
> {
> printf (_("\
> -Usage: %s [NUMBER]...\n\
> - or: %s OPTION\n\
> +Usage: %s [OPTION] [NUMBER]...\n\
> "),
> - program_name, program_name);
> + program_name);
> fputs (_("\
> Print the prime factors of each specified integer NUMBER. If none\n\
> are specified on the command line, read them from standard input.\n\
> \n\
> "), stdout);
> + fputs ("\
> + -h, --exponents print factors in the form p^e\n\
> + rather than repeating the prime p, e times.\n\
> +", stdout);
It doesn't say that ^e is omitted of e==1.
I think the most concise form is:
"print factors in form p^e unless e is 1"
I think it's clear what p^e means, or otherwise the reader can consult
the Texinfo manual.
> fputs (HELP_OPTION_DESCRIPTION, stdout);
> fputs (VERSION_OPTION_DESCRIPTION, stdout);
> emit_ancillary_info (PROGRAM_NAME);
> @@ -2593,10 +2611,14 @@ main (int argc, char **argv)
> atexit (lbuf_flush);
>
> int c;
> - while ((c = getopt_long (argc, argv, "", long_options, NULL)) != -1)
> + while ((c = getopt_long (argc, argv, "h", long_options, NULL)) != -1)
> {
> switch (c)
> {
> + case 'h': /* NetBSD used -h for this functionality first. */
> + print_exponents = true;
> + break;
> +
> case DEV_DEBUG_OPTION:
> dev_debug = true;
> break;
> diff --git a/tests/factor/create-test.sh b/tests/factor/create-test.sh
> index be8248792..9c7d20422 100755
> --- a/tests/factor/create-test.sh
> +++ b/tests/factor/create-test.sh
> @@ -66,6 +66,7 @@ case $t in
> t34) set ${q}956336 ${q}958335
> d1ae6bc712d994f35edf55c785d71ddf31f16535 ;;
> t35) set ${q}958336 ${q}960335
> 2374919a89196e1fce93adfe779cb4664556d4b6 ;;
> t36) set ${q}960336 ${q}962335
> 569e4363e8d9e8830a187d9ab27365eef08abde1 ;;
> + t37) set -- 0 10000000 8c4d3f021ac89fa0f7ce21857e16474549f83417
> -h ;;
> *)
> echo "$0: error: unknown test: '$test_name' -> '$t'" >&2
> exit 1
> @@ -80,4 +81,5 @@ exec sed \
> -e "s/__START__/$1/" \
> -e "s/__END__/$2/" \
> -e "s/__CKSUM__/$3/" \
> + -e "s/__OPTS__/$4/" \
> -e "s!__TEMPLATE__!$TEMPLATE!" "$template"
I'm not sure if it's the best choice to complicate this test with $4.
And it is only run when RUN_VERY_EXPENSIVE_TESTS=yes; this limits regular
test coverage.
I'd rather add a simple, separate test which nicely visualizes how
the -h (and --exponent!) option(s) work. Finally, there's no need to
exercise 10 million numbers for this IMO.
WDYT?
> diff --git a/tests/factor/run.sh b/tests/factor/run.sh
> index 4a1dbb01b..a95e9b6d3 100755
> --- a/tests/factor/run.sh
> +++ b/tests/factor/run.sh
> @@ -23,12 +23,13 @@ print_ver_ factor seq sha1sum
> START=__START__
> END=__END__
> CKSUM=__CKSUM__
> + OPTS=__OPTS__
>
> test "$START" = '__ST''ART__' && skip_ 'ignoring factor test template'
>
> echo "$CKSUM -" > exp
>
> f=1
> -seq $START $END | factor | sha1sum -c --status exp && f=0
> +seq $START $END | factor $OPTS | sha1sum -c --status exp && f=0
>
> Exit $f
> diff --git a/tests/local.mk b/tests/local.mk
> index 0f7778619..ff919bfc5 100644
> --- a/tests/local.mk
> +++ b/tests/local.mk
> @@ -739,7 +739,7 @@ factor_tests = \
> $(tf)/t20.sh $(tf)/t21.sh $(tf)/t22.sh $(tf)/t23.sh $(tf)/t24.sh \
> $(tf)/t25.sh $(tf)/t26.sh $(tf)/t27.sh $(tf)/t28.sh $(tf)/t29.sh \
> $(tf)/t30.sh $(tf)/t31.sh $(tf)/t32.sh $(tf)/t33.sh $(tf)/t34.sh \
> - $(tf)/t35.sh $(tf)/t36.sh
> + $(tf)/t35.sh $(tf)/t36.sh $(tf)/t37.sh
>
> $(factor_tests): $(tf)/run.sh $(tf)/create-test.sh
> $(AM_V_GEN)$(MKDIR_P) $(tf)
> --
> 2.26.2
Have a nice day,
Berny
