Hi Job,
Job Snijders wrote on Wed, Jul 04, 2018 at 02:09:56PM +0200:
> Following some back and forth on how disklabel output should be
> formatted, I proposed to Kenneth to extend the column(1) utility.
> All that was missing is the ability to right justify.
I'm not totally sure we should add the feature, but maybe you are
right that it's useful enough, and column(1) still remains simple
enough. We have to pay attention not to turn it into a pig
resembling rs(1), though.
> Patch courtesy of Kenneth R Westerback. OK?
No. I see at least two aspects of the patch that are clearly
not yet OK, see inline.
> Index: column.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/column/column.1,v
> retrieving revision 1.18
> diff -u -p -r1.18 column.1
> --- column.1 24 Oct 2016 13:53:05 -0000 1.18
> +++ column.1 4 Jul 2018 10:27:54 -0000
> @@ -40,6 +40,7 @@
> .Nm column
> .Op Fl tx
> .Op Fl c Ar columns
> +.Op Fl r Op Ar list
This is unacceptable. A few programs exist that take options with
optional arguments, but we certainly do not want to add new ones
because their syntax and semantics is inherently ambiguous and error
prone. POSIX also discourages it in general.
An option can either take no argument or require an argument,
so make a decision in that respect. If you opt for requiring
an argument, then you can use a cut(1)-like syntax:
cut -b 2-3 # bytes 2 and 3
cut -b 2- # all but the first byte
cut -b -2 # the first two bytes
cut -b 1- # all bytes, not cutting away anything
cut -b -2,4- # all but the third byte
[...]
> Index: column.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/column/column.c,v
> retrieving revision 1.26
> diff -u -p -r1.26 column.c
> --- column.c 22 Jun 2018 12:27:00 -0000 1.26
> +++ column.c 4 Jul 2018 10:28:00 -0000
[...]
> + rflag = 0; tflag = xflag = 0;
That looks inconsistent. s/0; //
[...]
> @@ -207,18 +213,69 @@ print(void)
> puts(table[row]->content);
> }
>
> +int
> +rightjustify(const char *rcols, const int col)
> +{
> + const char *errstr;
> + char c, *num, *temp;
> + long long ch, rangestart;
> + unsigned int i;
> +
> + if (rcols == NULL)
> + return 1;
> +
> + temp = strdup(rcols);
> + num = temp;
> + rangestart = -1;
> +
> + c = 0;
> + for (i = 0; i <= strlen(rcols); i++) {
> + ch = temp[i];
> + if (ch == ',' || ch == '-')
> + temp[i] = '\0';
> + if (temp[i] != '\0')
> + continue;
> +
> + c = strtonum(num, 1, INT_MAX, &errstr);
This is not acceptable either.
Your algorithm is of the order
O(rows) * O(columns) * O(strlen(rcols))
and then calling strdup(3) on the quadratic level and strtonum(3)
inside the cubic loop. Even though performance optimization is
often over-valued, that is not an excuse for selecting a basic
algorithm with an excessively high algorithmic order.
You really have to do argument parsing once before entering the
quadratic main loop rather than redoing it from scratch for each
and every table cell.
[...]
> void
> -maketbl(void)
> +maketbl(const int rflag, const char *rcols)
> {
> struct field **row;
> int col;
>
> for (row = table; entries--; ++row) {
> - for (col = 0; (*row)[col + 1].content != NULL; ++col)
[...]
> + for (col = 0; (*row)[col].content != NULL; ++col) {
Here, you are losing the feature that column(1) refrains from
printing trailing blanks.
I'm putting the audit on hold at this points because fixing the
issues found so far is going to change the patch substantially.
Yours,
Ingo
