Re: [PATCH 1/2] factor: add option for printing in x^y format

[Bernhard Voelker]

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.villemoes@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