On 18/07/15 06:06, Assaf Gordon wrote: > Hello Pádraig, > > On Jul 15, 2015, at 21:25, Pádraig Brady <[email protected]> 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 <[email protected]> > 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.
