Re: alternative form flag with zero octal value

[Rich Felker <dalias@libc.org>]

On Thu, Jan 11, 2018 at 08:36:23PM +0100, Jens Gustedt wrote:
> Hello Rich,
> 
> On Thu, 11 Jan 2018 13:55:34 -0500 Rich Felker <dalias@libc.org> wrote:
> 
> > Yes, there is a good deal of stuff in that file that, in hindsight, I
> > would do somewhat differently. A few of the worst things you noted
> > could be 'fixed' trivially with gotos
> 
> No, I don't think this is necessary. I don't mind jumping in the
> middle of an "if (0)" branch, as long as it is indented correcly :)

I'm not sure we're on the same page about what's bad then. I think

		if (1) something; else
	case bar:
		something_else;

is gratuitously less readable than:

		something;
		goto shared_tail;
	case bar:
		something_else;
	shared_tail:

Ideally we wouldn't need to do either, but sometimes it's hard to
organized the code in a way that's not gratuitously redundant (at the
source level) or gratuitously larger without stuff like this.

> > or compound statatements and
> > probably should be. I don't want a big patch for this file that's hard
> > to review/validate though; the return on time spent is just too small.
> > If anyone does want to push me on making improvements here, small
> > isolated/individual changes that can easily be seen to be correct are
> > the way to go.
> 
> Just a patch for white space and indentation would be easy, I think.

I'm not a fan of patches that just recreate the output of an indention
utility.

>      - have break at the end of case lines, not at the start

I guess you mean the pop_arg function? It's written the way it is
because that's the form that makes it the easiest to see what's
happening in each case and what's different between them. Putting the
break on a separate line would make it take up a lot more space and
lower the visual SNR. Putting it at the end of the line (without a lot
of alignment space; such space could be an alternative option) would
butt rendundant text up against the part that differs, making it
harder to read too.

One appproach that might satisfy us both is using a macro, something
like:

#define POP_CASE(a,b,c) case a: arg->b = va_arg(*ap, c); break;

obviously with more meaningful names for the args. Then each line of
the switch only contains meaningful information.

>      - start a newline before else
>      - ident according to musl's coding style

I think a misunderstanding/different expectation you and perhaps
others have is that a "coding style" is a deterministic function from
source token sequences (and possibly some additional metadata like
presence of blank lines) to a canonical source file. Some projects do
things this way, but it's never been something I've liked. To me,
"coding style" means a set of formatting constraints that should
usually be met, unless there's a good reason to break them, but which
don't determine a unique form, plus guidelines for how to favor
readability when there's ambiguity about the best form.

> As long as git diff --color-words shows nothing, this should be fine.
> 
> I could such a patch, if you want.

I'd really rather spend time reviewing & merging functional patches
and implementing new things than discussing coding style. As I said
before, if there are individual/isolated changes that can be accepted
or deferred or rejected without requiring a long thread of v2, v3,
..., v10 patches until we agree on which combination of changes is
right, I'm happy to commit the ones I agree with right away, but I'd
still rather be spending time on something that feels more productive.

> Replacing comma operators by compound statements would be a bit more
> work.  We could delay that to when somebody touches that file for more
> serious reasons.

Perhaps. Sometimes I'm not sure whether I prefer changing this sort of
thing at the same time as a functional patch or doing it preemptively
so that later functional changes are slimmer and easier to understand.

Rich