All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jesse Barnes <jbarnes@virtuousgeek.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-kernel@vger.kernel.org,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>
Subject: Re: [git pull] PCI pull request for 2.6.27
Date: Wed, 16 Jul 2008 16:54:55 -0700	[thread overview]
Message-ID: <200807161654.55699.jbarnes@virtuousgeek.org> (raw)
In-Reply-To: <alpine.LFD.1.10.0807161647470.2835@woody.linux-foundation.org>

On Wednesday, July 16, 2008 4:50 pm Linus Torvalds wrote:
> On Wed, 16 Jul 2008, Jesse Barnes wrote:
> > Miklos Vajna (1):
> >       x86/PCI: janitor work in irq.c
>
> Please don't take patches like this.
>
> If it's janitor work, the end result should be better. But it's not. This
> patch is full of stuff like
>
> -	for(addr = (u8 *) __va(0xf0000); addr < (u8 *) __va(0x100000); addr +=
> 16) { +	for (addr = (u8 *) __va(0xf0000); addr < (u8 *) __va(0x100000);
> +		addr += 16) {
>  		rt = pirq_check_routing_table(addr);
>
> Which just brings negative value. The code is _harder_ to look at, not
> easier.
>
> The 80-character limit is less important than making code look obvious and
> indentation being readable. Splitting the for(;;) loop just made the
> indentation look like total crap.
>
> I'm fixing it up (since it also caused trivial conflicts), but I'd ask
> people to just ignore that sh*t-for-brains that is the long-line warning
> when trying to fix it may silence a warning, but results in worse code!

Yeah I noticed that too when I did the merge (and in the original patch), 
though on my 80 char display it looked like a wash in terms of readability to 
me, but even in that case I should have rejected it as noise.  Sorry about 
that.

Jesse

  reply	other threads:[~2008-07-16 23:55 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-07-16 23:39 [git pull] PCI pull request for 2.6.27 Jesse Barnes
2008-07-16 23:50 ` Linus Torvalds
2008-07-16 23:54   ` Jesse Barnes [this message]
2008-07-17  0:23     ` Maciej W. Rozycki
2008-07-17  0:38       ` Linus Torvalds
2008-07-17  0:28 ` Linus Torvalds
2008-07-21 13:57 ` [patch] acpi: fix crash in core ACPI code, triggered by CONFIG_ACPI_PCI_SLOT=y Ingo Molnar
2008-07-21 18:38   ` Andi Kleen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=200807161654.55699.jbarnes@virtuousgeek.org \
    --to=jbarnes@virtuousgeek.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.