[PATCH] Add a "sort role" for sorting the dive list

Linus Torvalds torvalds at linux-foundation.org
Wed May 29 14:19:35 PDT 2013


On Thu, May 30, 2013 at 6:06 AM, Dirk Hohndel <dirk at hohndel.org> wrote:
>
> Wait one minute and pull.
>
> It is ALL WORKING. Beautifully.

Well, almost. The sorting works again, but the sorting marker doesn't
show up. So you don't actually visually see which column things are
sorted by any more.

On a code style issue, I have to say that the

  switch (type) {
   case X: ret = ... ; break;
   }
   return ret;

coding style is just retarded. The comment says "makes debugging a
little easier" which I don't understand at all. It just makes the code
harder to read when you have to look at the very end of the line (that
is so long that it may not even fit in your window) to know if a case
statement breaks out or not.

The whole point of that format is that it's single-line and dense, but
exactly *because* it is dense, it needs to not have any subtle issues
like this.

So the *sane* format is very much

   case X:   return ...;

which makes the code flow very easy to see. There is never any subtle
syntax issues, question about whether something falls through or not,
and you don't have to visually scan to the end of the line. You see
exactly what that dense block of code does. Every single case
statement unambiguously returns a value, there's no question about
whether something falls through or what else happens in the function.

If something like this needs debugging, then there's something wrong
there. Maybe those thing could be split up into multiple functions, so
that instead of having one function that does nested case-statements
(role vs column), a function would do just one thing
("sorting_role_data()" for the SORT_ROLE thing etc).

              Linus


More information about the subsurface mailing list