Re: [PATCH] Detect mmconfig on nVidia MCP55

[Ingo Molnar]

* Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx> wrote:

> On Wednesday 04 February 2009 17:04:40 Ingo Molnar wrote:
> > 2) Please use vertical spaces when initializing structure fields. Instead
> > of the messy looking (and over-long-line generating) construct of:
> >
> >         pci_mmcfg_config[0].address = (extcfg & 0x00007fff) << 25;
> >         pci_mmcfg_config[0].pci_segment = 0;
> >         pci_mmcfg_config[0].start_bus_number = 0;
> >         pci_mmcfg_config[0].end_bus_number = (1 << (8 - ((extcfg >> 28) &
> > 3))) - 1; pci_mmcfg_config_num = 1;
> >
> >    You will get something like:
> >
> >         config->address                 = (extcfg & 0x00007fff) << 25;
> >         config->pci_segment             = 0;
> >         config->start_bus_number        = 0;
> >         config->end_bus_number          = (1 << (8 - ((extcfg >> 28) &
> > 3)));
> >
> >         pci_mmcfg_config = config;
> >         pci_mmcfg_config_num = 1;
> >
> >    Which makes it more structured, more reviewable - and more pleasant to
> >    look at as well.

It is arch/x86/ and scheduler / etc. policy for new code - and we follow 
that principle when we clean up code as well.

Same goes for static structure initializers. We do:

 static const struct file_operations perf_fops = {
	.release		= perf_release,
	.read			= perf_read,
	.poll			= terf_poll,
	.unlocked_ioctl		= perf_ioctl,
	.compat_ioctl		= perf_ioctl,
 };

And not:

 static const struct file_operations perf_fops = {
	.release = perf_release,
	.read = perf_read,
	.poll = perf_poll,
	.unlocked_ioctl	= perf_ioctl,
	.compat_ioctl = pert_ioctl,
 };

Beyond the prettiness and fun to look at factor, there are other advantages 
of vertical spaces as well:

Trivia: you play the role of the code reviewer. I have hidden two typos in 
the initializers above, one in each initializer block. The typos are of the 
same type but are in difference places so both need to be found 
individually.

Try to find the typos, and record the number of seconds it took for you to 
find them. Which typo took less time to find?

I'd suspect that for most people block#1 is the winner.

[ Note, you have a look at it as real source code with fixed width fonts and 
  you'll see the difference. You seem to be using KMail or so: there's weird 
  line wraps and other corruption of the code in your quote above - have you 
  really looked at the real code how it looks like to developers? ]

> I find it a matter of personal preference whether it is more pleasant to 
> look at and whether it is more or less readable.

Is your argument that to you it is less pleasant to look at?

Differences in taste is OK in borderline issues, but i think this one is far 
from being borderline, it's a basic typographic and aesthetic principle.

I'm not sure vertical spaces can be properly described via more rigid 
CodingStyle rules - for example vertical spaces look ugly if there's just 
two field initializations for example - but in the above case they are 
clearly the right thing to do.

	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/