Re: numfmt: minor improvements to --field parsing

[Pádraig Brady]

On 18/07/15 06:06, Assaf Gordon wrote:
> Hello Pádraig,
> 
> On Jul 15, 2015, at 21:25, Pádraig Brady <address@hidden> wrote:
>> Given the overlap it would be best to move the shared code
>> (and any associated global vars) to a set-fields.c module
> 
> Attached is a better draft, doing just that.
> 
> Regarding the implementation - there are few stylistic decisions to be made.
> I'm happy to hear opinions:
> 
>> FATAL_ERROR() or error() could be used, but I'd have a very
>> slight preference for error().

+1

> 
> I've currently kept FATAL_ERROR, because it also calls 'usage()' which adds 
> the 'try $prog --help for more information' message.
> If anything, this keeps the existing behavior of 'cut' intact, while 'numfmt' 
> is new enough so there's no real harm in changing its error reporting.
> 
>> For divergences you could
>> key on an extern int field_mode, initialized globally
>> in both cut.c and numfmt.c
> 
> Instead of a global variable, I've added an 'options' bitfield to the 
> 'set_fields' function (explained in set-fields.h).
> Is a global variable preferred?

No that's cool.

> Also,
> I've changed the error messages as little as possible, to be consistent with 
> 'cut' while still being mostly generic and usable by 'numfmt'.

perfect

> The second of the three patches contains only the changed wording of cut's 
> error messages in the cut.pl test module - easier to examine.
> 
> comments welcomed,
>  - assaf
> 
> 
> From c53ab1f18f429923f202125bea27b1276d7b9f87 Mon Sep 17 00:00:00 2001
> From: Assaf Gordon <address@hidden>
> Date: Fri, 17 Jul 2015 23:30:30 -0400
> Subject: [PATCH 1/3] cut: refactor into set-fields module
>
> Extract the functionality of parsing --field=LIST into a separate
> module, to be used by other programs.
>
> * src/cut.c: move field parsing code from here ...
> * src/set-fields.{c,h}: ... to here.
>   set_fields(): generalize by supporting multiple parsing/reporting

s/set_fields():/(set_fields):/
Have a look at vc-chlog for generating templates:
http://meyering.net/links/

>   options.
>   struct range_pair: rename to field_range_pair.

s/struct range_pair/(struct range_pair)/

> @@ -793,13 +562,9 @@ main (int argc, char **argv)
>      FATAL_ERROR (_("suppressing non-delimited lines makes sense\n\
>  \tonly when operating on fields"));
>
> -  if (! set_fields (spec_list_string))
> -    {
> -      if (operating_mode == field_mode)
> -        FATAL_ERROR (_("missing list of fields"));
> -      else
> -        FATAL_ERROR (_("missing list of positions"));
> -    }
> +  set_fields (spec_list_string,
> +              ((operating_mode == field_mode)?0:SETFLD_ERRMSG_USE_POS)
> +              | (complement?SETFLD_COMPLEMENT:0) );

Spaces better around ? :

> diff --git a/src/set-fields.c b/src/set-fields.c
> new file mode 100644
> index 0000000..432c58b
> --- /dev/null
> +++ b/src/set-fields.c
> @@ -0,0 +1,309 @@
> +/* set-fields.c -- common functions for parsing field list
> +   Copyright (C) 2008-2015 Free Software Foundation, Inc.

Well set_fields() was there sinc ethe 1990s,
so probably best to just use 2015 here.
Ditto in the header.


> diff --git a/src/set-fields.h b/src/set-fields.h
> +/* field list parsing options */
> +enum
> +{
> +  SETFLD_ALLOW_DASH = 0x01,     /* allow single dash meaning 'all fields' */
> +  SETFLD_COMPLEMENT = 0x02,     /* complement the field list */
> +  SETFLD_ERRMSG_USE_POS = 0x04  /* when reporting errors, say 'position' 
> instead
> +                                   of 'field' (used with cut -b/-c) */
> +};

Nice.

> +             {EXIT=>1}, {ERR=>"$prog: field number " .
> +                              "'9918446744073709551616' is too 
> large\n$try"}],
> +

Should that be parameterized from getlimits?

Otherwise it looks good.

thanks!
Pádraig.