Re: [PATCH 1/5] lib/sort: Make swap functions more generic

[Geert Uytterhoeven]

Hi George,

On Fri, Mar 15, 2019 at 4:36 AM George Spelvin <lkml@xxxxxxx> wrote:
> >> swap_bytes / swap_4byte_words / swap_8byte_words
> >> swap_bytes / swap_ints / swap_longs
> >> swap_1 / swap_4 / swap_8
> >> Pistols at dawn?
>
> On Thu, 14 Mar 2019 at 22:59:55 +0300, Andrey Abramov wrote:
> > Yes, in my opinion, swap_bytes / swap_ints / swap_longs are the
> > most readable because we have both swap_ints and swap_longs functions
> > (in one file near each other), so I don't think that there will be
> > any confusion about size.
>
> Yes, that's what I thought.  They're three related but different
> functions, suffixed _bytes, _ints, and _longs.  What could the
> difference possibly be?  And if anyone has any lingering doubts,
> the functions are right there, with exquisitely clear comments.
>
> No to mention where they're used.  Is "is_aligned(base, size, 8)"
> remotely obscure?  Especially in context:
>
>                 if (is_aligned(base, size, 8))
>                         swap_func = swap_longs;
>                 else if (is_aligned(base, size, 4))
>                         swap_func = swap_ints;
>                 else
>                         swap_func = swap_bytes;
>
> What subtle and mysterious code.
>
> > But actually, it doesn't matter which name will you take, because
> > the meaning of each, in my opinion, is obvious enough, so I don't
> > mind about any of these options.
>
> I'm just amazed that this piece of bikeshedding is the most
> contentious thing about the patch series.
>
> I mean, if I'd named them:
>         llanfairpwllgwyngyll()
>         shravanabelagola()
>         zheleznodorozhny()
> or
>         peckish()
>         esuriant()
>         hungry()
> then yes, those would be bad names.
>
> I prefer the shorter _ints and _longs names, but this is just
> not a hill I want to die on.

Argument of least surprise: don't call something a duck if it's not
guaranteed to behave like a duck.

If I read "long", this triggers a warning flag in my head: be careful, this is
32-bit on 32-bit platforms, and 64-bit on 64-bit platforms.

There's a reason the newer I/O ioread{8,16,32} accessors use explicit
sizes, unlike the ancient x86-centric read[bwl]().

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds