public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Dan Carpenter <error27@gmail.com>
To: Xenofon Foukas <foukas.xenofon@gmail.com>
Cc: gregkh@suse.de, john.d.sheehan@gmail.com,
	u.kleine-koenig@pengutronix.de, arun.thomas@gmail.com,
	devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] Fix checkpatch issues in file s626.c
Date: Fri, 17 Dec 2010 16:20:37 +0300	[thread overview]
Message-ID: <20101217132037.GK1639@bicker> (raw)
In-Reply-To: <1292575893-17677-1-git-send-email-foukas.xenofon@gmail.com>

On Fri, Dec 17, 2010 at 10:51:33AM +0200, Xenofon Foukas wrote:
> This patch fixes line over 80 characters warnings
> in file s626.c

I don't really think it's worthwhile to do this.  In my opinion, the
original long lines are more readable than the new ones.

Long lines are a bad thing because they show that the coders don't know
what they are doing.  But chopping the lines into smaller lines is like
painting over a person who has small pox.  It doesn't fix the problem.

There are a couple other problems with this patch.

The subject should say:
[PATCH 2/2] Staging: comedi: Fix checkpatch issues in file s626.c

The patch changes places that say:
	DEBUG("foo");
to:
	printk(KERN_DEBUG "foo");

The trick with the DEBUG() macro is that it normally doesn't print
anything by default because it's compiled out.  This patch makes it
compiled in always.

You didn't describe that change in the changelog.

Also the indenting in the new patch isn't always the right.  The
multi-line commenting style isn't right.

regards,
dan carpenter


      reply	other threads:[~2010-12-17 13:20 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-12-17  8:51 [PATCH 2/2] Fix checkpatch issues in file s626.c Xenofon Foukas
2010-12-17 13:20 ` Dan Carpenter [this message]

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=20101217132037.GK1639@bicker \
    --to=error27@gmail.com \
    --cc=arun.thomas@gmail.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=foukas.xenofon@gmail.com \
    --cc=gregkh@suse.de \
    --cc=john.d.sheehan@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=u.kleine-koenig@pengutronix.de \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox