Re: [PATCH 2/3] x86-64: use relative 32-bit pointers in exceptiontables

[Ingo Molnar]

* Jan Beulich <JBeulich@xxxxxxxxxx> wrote:

> >>> On 17.02.11 at 18:25, Ingo Molnar <mingo@xxxxxxx> wrote:
> 
> > Nice patch. I've got a really small code readability nitpick:
> > 
> >> +#ifndef ex_insn /* until all architectures have this accessor */
> >> +#define ex_insn(x) (x)->insn
> >> +#endif
> > 
> >> +#else
> >> +#define swap_ex NULL
> >> +#endif
> > 
> > In the x86 architecture we tend to write this as:
> > 
> >> +#else
> >> +# define swap_ex NULL
> >> +#endif
> > 
> > So that the conditional structure stands out more, visually. (There might be 
> > more 
> > such cases in these patches as well.)
> 
> I can certainly fix this, but got a comment from (I think) Andrew
> Morton to do exactly the opposite quite some time ago, with the
> rationale that this indentation leads to more involved patches
> when further conditionals get added around them.

Well, the patch impact argument is a valid concern, but by that logic we should also 
drop the visual structure of other conditionals such as:

	if (x) {
		if (y)
			z;
		else
			k;
	} else {
		l;
	}

and write:

	if (x) {
	if (y)
	z;
	else
	k;
	} else {
	l;
	}

? I don't think so.

There might be other cases where marking CPP code this way looks ugly but in this 
patch it's clearly helpful to readability.

So i think for consistency's (and eyeball health's) sake lets bring as much 
meaningful geometric structure into source code as possible. Future patch size 
worries are secondary IMO.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/