On Tue, Dec 31, 2019 at 03:07:30PM -0800, Michael Forney wrote:
> On 2019-12-31, Richard Ipsum <richardip...@vx21.xyz> wrote:
> > ---
> > sort.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/sort.c b/sort.c
> > index 941b694..e974011 100644
> > --- a/sort.c
> > +++ b/sort.c
> > @@ -196,7 +196,8 @@ slinecmp(struct line *a, struct line *b)
> > x = strtod(col1.line.data, NULL);
> > y = strtod(col2.line.data, NULL);
> > res = (x < y) ? -1 : (x > y);
> > - } else if (kd->flags & (MOD_D | MOD_F | MOD_I)) {
> > + } else if ((kd->flags & MOD_D) | (kd->flags & MOD_F) |
> > + (kd->flags & MOD_I)) {
>
> This is functionally the same, right? Any reason for this change?
My mistake, it is the same, this change can be dropped.
>
> > res = skipmodcmp(&col1.line, &col2.line, kd->flags);
> > } else {
> > res = linecmp(&col1.line, &col2.line);
> > @@ -386,10 +387,13 @@ main(int argc, char *argv[])
> > usage();
> > } ARGEND
> >
> > - /* -b shall only apply to custom key definitions */
> > - if (TAILQ_EMPTY(&kdhead) && global_flags)
> > - addkeydef("1", global_flags & ~(MOD_STARTB | MOD_ENDB));
> > - addkeydef("1", global_flags & MOD_R);
> > + if (TAILQ_EMPTY(&kdhead)) {
> > + if (global_flags) {
> > + /* -b shall only apply to custom key definitions */
> > + addkeydef("1", global_flags & ~(MOD_STARTB | MOD_ENDB));
> > + }
> > + addkeydef("1", global_flags & MOD_R);
> > + }
> >
> > if (!argc) {
> > if (Cflag || cflag) {
>
> We use qsort to sort the lines, and the order of elements that compare
> equal when applied to qsort is unspecified, so I think this means that
> the output of sbase sort(1) could vary from libc to libc. POSIX says
> that `The order in which lines that still compare equal are written is
> unspecified`, so I don't think there is actually a conformance issue
> with that, but it is something to consider.
>
> I do see the problem with the top-level sort when -[cC] is used, so
> maybe another option is to only add it when we are not doing order
> checking.
>
> What are your thoughts on this?
I can see the argument for keeping the behaviour as is and special
casing for -c and -C.
I'll rework the patch then.
Thanks,
Richard
