Messages in this thread | | | Date | Fri, 18 Feb 2011 09:14:46 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH 2/3] x86-64: use relative 32-bit pointers in exception tables |
| |
* Jan Beulich <JBeulich@novell.com> wrote:
> >>> On 17.02.11 at 18:25, Ingo Molnar <mingo@elte.hu> 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
| |