* 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-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
