On Tue, 29 May 2007, Robert Hancock wrote:
>
> This path adds validation of the MMCONFIG table against the ACPI reserved
> motherboard resources.
Please fix the formatting of your code.
"for" and "if" are not functions, and they have a space before the
parenthesis.
And pretty much every single conditional in this patch is spread out over
two or more lines and has at least three different indentations. There's
something wrong here. Code can't look this bad and still be fine. Some of
this looks like random whitespace noise:
+ if(is_acpi_reserved(cfg->address,
+ cfg->address + size - 1))
+ printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved "
+ "in ACPI motherboard resources\n",
+ cfg->address);
+ else {
That's just horrid. Please try to make the code _look_ nicer.
For example, just making "is_acpi_reserved()" take a start/len thing
instead, would allow you to at least do
if (is_acpi_reserved(cfg->address, size)) {
printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved "
"in ACPI motherboard resources\n",
cfg->address);
} else {
...
(and has the braces rigth too - don't pair an unbraced "if ()" with a
braced "else" statement), and that together with making the body of the
for-loop a separate function would possibly make that code read a lot
better.
Same goes for this thing:
+ if((pci_probe & PCI_PROBE_CONF1) &&
+ e820_all_mapped(cfg->address,
+ cfg->address + size - 1,
+ E820_RESERVED))
+ printk(KERN_NOTICE "PCI: MCFG area at %Lx reserved in E820\n",
+ cfg->address);
+ else
+ goto reject;
there really is *not* a highly coveted prize for having the most different
indentation in the fewest possible lines of code!
Yeah, I realize that maybe this is nit-picking, but trying to read this
patch really does make you go blind. It violates so many coding standards
that it's almost impossible to read the code itself. It's made worse by
the fact that you then also used Thunderbird to send the patch, and had it
set for
Content-type: text/plain; charset=ISO-8859-1; format=flowed
where that "format=flowed" means that basically no mail-client will be
able to read it sanely (because a lot of them will re-flow the text), and
you have to save it to a file before you can even comment on it.
Gaah. See
http://mbligh.org/linuxdocs/Email/Clients/Thunderbird
where the most important sentence is "Thunderbird is written by aged whore
monkeys stoned on crack". But it also talks about how to disable that
idiotic format=flowed for patches, and how to make sure it's not wrapping.
But as mentioned, your patch itself had some whitespace issues even aside
from the regular Thunderbird breakage.
Linus
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
