[PATCH 00/10] Non-printk continuation line fixes

[PATCH 00/10] Non-printk continuation line fixes

[Joe Perches]

Some more fixes to formats with continuation lines

Joe Perches (10):
  arch/ia64: Fix continuation line formats
  drivers/ata: Fix continuation line formats
  drivers/gpu/drm/i915/intel_bios.c: Fix continuation line formats
  drivers/hwmon/vt8231.c: Fix continuation line formats
  drivers/net/davinci_emac.c: Fix continuation line formats
  drivers/scsi/qla2xxx/qua_os.c: Fix continuation line formats
  drivers/spi/spi_s3c64xx.c: Fix continuation line formats
  drivers/staging: Fix continuation line formats
  drivers/video/via: Fix continuation line formats
  sound/soc/s3c24xx/s2c-pcm.c: Fix continuation line formats

 arch/ia64/kvm/mmio.c                   |    4 ++--
 arch/ia64/kvm/vcpu.c                   |    4 ++--
 drivers/ata/pata_at91.c                |    4 ++--
 drivers/gpu/drm/i915/intel_bios.c      |    3 +--
 drivers/hwmon/vt8231.c                 |    3 +--
 drivers/net/davinci_emac.c             |    3 +--
 drivers/scsi/qla2xxx/qla_os.c          |    4 ++--
 drivers/spi/spi_s3c64xx.c              |   11 +++++------
 drivers/staging/rtl8192su/r8192S_phy.c |   16 ++++++++--------
 drivers/staging/sep/sep_driver.c       |    5 +++--
 drivers/staging/slicoss/slicoss.c      |    6 ++----
 drivers/video/via/dvi.c                |    8 ++++----
 drivers/video/via/lcd.c                |   12 ++++++------
 sound/soc/s3c24xx/s3c-pcm.c            |    3 +--
 14 files changed, 40 insertions(+), 46 deletions(-)


------------------------------------------------------------------------------
The Planet: dedicated and managed hosting, cloud storage, colocation
Stay online with enterprise data centers and the best network in the business
Choose flexible plans and management services without long-term contracts
Personal 24x7 support from experience hosting pros just a phone call away.
http://p.sf.net/sfu/theplanet-com
--

[PATCH 02/10] drivers/ata: Fix continuation line formats

[Joe Perches]

String constants that are continued on subsequent lines with \
are not good.

Signed-off-by: Joe Perches <joe@perches.com>
---
 drivers/ata/pata_at91.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c
index 41c94b1..d222c0d 100644
--- a/drivers/ata/pata_at91.c
+++ b/drivers/ata/pata_at91.c
@@ -153,8 +153,8 @@ static void pata_at91_set_piomode(struct ata_port *ap, struct ata_device *adev)
 	/* Compute ATA timing and set it to SMC */
 	ret = ata_timing_compute(adev, adev->pio_mode, &timing, 1000, 0);
 	if (ret) {
-		dev_warn(ap->dev, "Failed to compute ATA timing %d, \
-				set PIO_0 timing\n", ret);
+		dev_warn(ap->dev, "Failed to compute ATA timing %d, set PIO_0 timing\n",
+			 ret);
 		set_smc_timing(ap->dev, info, &initial_timing);
 	} else {
 		set_smc_timing(ap->dev, info, &timing);
-- 
1.6.6.rc0.57.gad7a

Re: [PATCH 02/10] drivers/ata: Fix continuation line formats

[Mark Lord]

Joe Perches wrote:
> String constants that are continued on subsequent lines with \
> are not good.
> 
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  drivers/ata/pata_at91.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c
> index 41c94b1..d222c0d 100644
> --- a/drivers/ata/pata_at91.c
> +++ b/drivers/ata/pata_at91.c
> @@ -153,8 +153,8 @@ static void pata_at91_set_piomode(struct ata_port *ap, struct ata_device *adev)
>  	/* Compute ATA timing and set it to SMC */
>  	ret = ata_timing_compute(adev, adev->pio_mode, &timing, 1000, 0);
>  	if (ret) {
> -		dev_warn(ap->dev, "Failed to compute ATA timing %d, \
> -				set PIO_0 timing\n", ret);
> +		dev_warn(ap->dev, "Failed to compute ATA timing %d, set PIO_0 timing\n",
> +			 ret);
..

NAK.  Just stick the ret); part onto the same line and be done with it.

-ml

Re: [PATCH 02/10] drivers/ata: Fix continuation line formats

[Jeff Garzik]

On 02/03/2010 12:21 PM, Mark Lord wrote:
> Joe Perches wrote:
>> String constants that are continued on subsequent lines with \
>> are not good.
>>
>> Signed-off-by: Joe Perches <joe@perches.com>
>> ---
>> drivers/ata/pata_at91.c | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c
>> index 41c94b1..d222c0d 100644
>> --- a/drivers/ata/pata_at91.c
>> +++ b/drivers/ata/pata_at91.c
>> @@ -153,8 +153,8 @@ static void pata_at91_set_piomode(struct ata_port
>> *ap, struct ata_device *adev)
>> /* Compute ATA timing and set it to SMC */
>> ret = ata_timing_compute(adev, adev->pio_mode, &timing, 1000, 0);
>> if (ret) {
>> - dev_warn(ap->dev, "Failed to compute ATA timing %d, \
>> - set PIO_0 timing\n", ret);
>> + dev_warn(ap->dev, "Failed to compute ATA timing %d, set PIO_0
>> timing\n",
>> + ret);
> ..
>
> NAK. Just stick the ret); part onto the same line and be done with it.

Agreed...

Re: [PATCH 02/10] drivers/ata: Fix continuation line formats

[Sergey Matyukevich]

Hello,
Thanks for attention and fix. For me Joe's fix is fine. 
But if you would prefer one-line version:

diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c
index 41c94b1..7ce9595 100644
--- a/drivers/ata/pata_at91.c
+++ b/drivers/ata/pata_at91.c
@@ -153,8 +153,7 @@ static void pata_at91_set_piomode(struct ata_port *ap, struct ata_device *adev)
 	/* Compute ATA timing and set it to SMC */
 	ret = ata_timing_compute(adev, adev->pio_mode, &timing, 1000, 0);
 	if (ret) {
-		dev_warn(ap->dev, "Failed to compute ATA timing %d, \
-				set PIO_0 timing\n", ret);
+		dev_warn(ap->dev, "Failed to compute ATA timing %d, set PIO_0 timing\n", ret);
 		set_smc_timing(ap->dev, info, &initial_timing);
 	} else {
 		set_smc_timing(ap->dev, info, &timing);


> > Joe Perches wrote:
> >> String constants that are continued on subsequent lines with \
> >> are not good.
> >>
> >> Signed-off-by: Joe Perches <joe@perches.com>
> >> ---
> >> drivers/ata/pata_at91.c | 4 ++--
> >> 1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/ata/pata_at91.c b/drivers/ata/pata_at91.c
> >> index 41c94b1..d222c0d 100644
> >> --- a/drivers/ata/pata_at91.c
> >> +++ b/drivers/ata/pata_at91.c
> >> @@ -153,8 +153,8 @@ static void pata_at91_set_piomode(struct
> >> ata_port *ap, struct ata_device *adev)
> >> /* Compute ATA timing and set it to SMC */
> >> ret = ata_timing_compute(adev, adev->pio_mode, &timing, 1000, 0);
> >> if (ret) {
> >> - dev_warn(ap->dev, "Failed to compute ATA timing %d, \
> >> - set PIO_0 timing\n", ret);
> >> + dev_warn(ap->dev, "Failed to compute ATA timing %d, set PIO_0
> >> timing\n",
> >> + ret);
> > ..
> >
> > NAK. Just stick the ret); part onto the same line and be done with
> > it.
> 
> Agreed...
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/10] drivers/ata: Fix continuation line formats

[Joe Perches]

On Wed, 2010-02-03 at 12:21 -0500, Mark Lord wrote:
> Joe Perches wrote:
> > String constants that are continued on subsequent lines with \
> > are not good.
[]
> > -		dev_warn(ap->dev, "Failed to compute ATA timing %d, \
> > -				set PIO_0 timing\n", ret);
> > +		dev_warn(ap->dev, "Failed to compute ATA timing %d, set PIO_0 timing\n",
> > +			 ret);
> NAK.  Just stick the ret); part onto the same line and be done with it.

:resend that I mistakenly sent only to Mark, with a couple more comments

I disagree.

Look at the long line wrapping style of the rest of the file
and most of the kernel source.  No additional arguments to
functions are generally used after column 80.

If it's ever agreed that all lines > 80 cols are OK or
new args after column 80 are OK, then sure.  Until then,
if you want it, you should do it.

cheers, Joe

$ grep -A 1 "\bdev_" drivers/ata/pata_at91.c 
	dev_dbg(dev, "ATA timings: nrd_setup = %lu nrd_pulse = %lu nrd_cycle = %lu\n",
			nrd_setup, nrd_pulse, read_cycle);
	dev_dbg(dev, "ATA timings: nwe_setup = %lu nwe_pulse = %lu nwe_cycle = %lu\n",
			nwe_setup, nwe_pulse, write_cycle);
	dev_dbg(dev, "ATA timings: ncs_read_setup = %lu ncs_read_pulse = %lu\n",
			ncs_read_setup, ncs_read_pulse);
	dev_dbg(dev, "ATA timings: ncs_write_setup = %lu ncs_write_pulse = %lu\n",
			ncs_write_setup, ncs_write_pulse);
--
		dev_warn(ap->dev, "Failed to compute ATA timing %d, set PIO_0 timing\n",
			 ret);
--
		dev_err(&pdev->dev, "invalid number of resources\n");
		return -EINVAL;
--
		dev_err(dev, "failed to get mem resource\n");
		return -EINVAL;
--
		dev_err(dev, "failed to allocate memory for private data\n");
		return -ENOMEM;
--
		dev_err(dev, "failed to get access to mck clock\n");
		return -ENODEV;
--
		dev_err(dev, "failed to map IO base\n");
		ret = -ENOMEM;
--
		dev_err(dev, "failed to map CTL base\n");
		ret = -ENOMEM;
--
	struct ata_host *host = dev_get_drvdata(&pdev->dev);
	struct at91_ide_info *info;

Re: [PATCH 02/10] drivers/ata: Fix continuation line formats

[Krzysztof Halasa]

Joe Perches <joe@perches.com> writes:

> If it's ever agreed that all lines > 80 cols are OK or
> new args after column 80 are OK, then sure.

It's been already agreed they are OK for any code.
There is no problem with complexity there, is there?
-- 
Krzysztof Halasa

Re: [PATCH 02/10] drivers/ata: Fix continuation line formats

[Joe Perches]

On Thu, 2010-02-04 at 02:39 +0100, Krzysztof Halasa wrote: 
> Joe Perches <joe@perches.com> writes:
> It's been already agreed they are OK for any code.
> There is no problem with complexity there, is there?

There's no complexity problem.
You're welcome to do it however you chose.
Just as I did.

cheers, Joe

Re: [PATCH 02/10] drivers/ata: Fix continuation line formats

[Mark Lord]

Joe Perches wrote:
> On Wed, 2010-02-03 at 12:21 -0500, Mark Lord wrote:
>> Joe Perches wrote:
>>> String constants that are continued on subsequent lines with \
>>> are not good.
> []
>>> -		dev_warn(ap->dev, "Failed to compute ATA timing %d, \
>>> -				set PIO_0 timing\n", ret);
>>> +		dev_warn(ap->dev, "Failed to compute ATA timing %d, set PIO_0 timing\n",
>>> +			 ret);
>> NAK.  Just stick the ret); part onto the same line and be done with it.
> 
> :resend that I mistakenly sent only to Mark, with a couple more comments
> 
> I disagree.
> 
> Look at the long line wrapping style of the rest of the file
> and most of the kernel source.  No additional arguments to
> functions are generally used after column 80.
> 
> If it's ever agreed that all lines > 80 cols are OK or
> new args after column 80 are OK, then sure.  Until then,
> if you want it, you should do it.
..

You are the code nanny trying to get this patch in.  Not me.

-ml

Re: [PATCH 02/10] drivers/ata: Fix continuation line formats

[Andrew Morton]

On Wed, 03 Feb 2010 23:59:47 -0500 Mark Lord <kernel@teksavvy.com> wrote:

> Joe Perches wrote:
> > On Wed, 2010-02-03 at 12:21 -0500, Mark Lord wrote:
> >> Joe Perches wrote:
> >>> String constants that are continued on subsequent lines with \
> >>> are not good.
> > []
> >>> -		dev_warn(ap->dev, "Failed to compute ATA timing %d, \
> >>> -				set PIO_0 timing\n", ret);
> >>> +		dev_warn(ap->dev, "Failed to compute ATA timing %d, set PIO_0 timing\n",
> >>> +			 ret);
> >> NAK.  Just stick the ret); part onto the same line and be done with it.
> > 
> > :resend that I mistakenly sent only to Mark, with a couple more comments
> > 
> > I disagree.
> > 
> > Look at the long line wrapping style of the rest of the file
> > and most of the kernel source.  No additional arguments to
> > functions are generally used after column 80.
> > 
> > If it's ever agreed that all lines > 80 cols are OK or
> > new args after column 80 are OK, then sure.  Until then,
> > if you want it, you should do it.
> ..
> 
> You are the code nanny trying to get this patch in.  Not me.
> 

It's a runtime bug.  Are "code nannies" supposed to "try" to fix
runtime bugs in libata while others just sit there and insult them?

Re: [PATCH 02/10] drivers/ata: Fix continuation line formats

[Mark Lord]

Andrew Morton wrote:
> On Wed, 03 Feb 2010 23:59:47 -0500 Mark Lord <kernel@teksavvy.com> wrote:
>> Joe Perches wrote:
>>> If it's ever agreed that all lines > 80 cols are OK or
>>> new args after column 80 are OK, then sure.  Until then,
>>> if you want it, you should do it.
>> ..
>>
>> You are the code nanny trying to get this patch in.  Not me.
>>
> 
> It's a runtime bug.  Are "code nannies" supposed to "try" to fix
> runtime bugs in libata while others just sit there and insult them?
..

Oh, there's a service affecting bug there?
Well, I guess that means we should accept a shoddy patch, then.  :)

But seriously, I didn't mean anything bad by it, just that
it ought to be simple for the original sender to just reformat it
and resubmit.  Just like everyone else here does on a daily basis.

No point in making the code harder to read/understand for no good reason.

But if these patches are robot generated (by a code scan tool or something),
then the original sender (Joe) might have many others in the queue
throughout the kernel, which could explain the reluctance here.

Really, though the best solution would be for Jeff to just edit
that one line as he integrates the patch, like Linus would do.
I imagine Jeff's already done that by now.  ;)

Cheers

Re: [PATCH 02/10] drivers/ata: Fix continuation line formats

[Joe Perches]

On Wed, 2010-02-03 at 23:59 -0500, Mark Lord wrote:
> You are the code nanny trying to get this patch in.  Not me.

Nope, I'm just the guy that identified the defect
and proposed a patch.  You can fix it differently
if you want.  Jeff could fix it differently as well.
I don't care that much.  I don't own a device that
uses this controller.  I'll never see the message.
Perhaps no one ever will.

Maybe Sergey Matyukevich, the original author, wants
to fix it up in another way.
http://patchwork.kernel.org/patch/76246/

cheers, Joe

Re: [PATCH 02/10] drivers/ata: Fix continuation line formats

[Mark Lord]

Joe Perches wrote:
> If it's ever agreed that all lines > 80 cols are OK or
> new args after column 80 are OK, then sure.  Until then,
> if you want it, you should do it.
..

In case you missed it, it *has* already been "agreed" that
there's nothing magical about 80 anymore.  



-------- Original Message --------
Subject: Re: [PATCH] ptrace: checkpatch fixes
Date: Wed, 8 Apr 2009 10:19:36 -0700 (PDT)
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Roland McGrath <roland@redhat.com>
CC: Andrew Morton <akpm@linux-foundation.org>,	linux-kernel@vger.kernel.org
References: <20090408062106.39EE0FC3E5@magilla.sf.frob.com>


On Tue, 7 Apr 2009, Roland McGrath wrote:
>
> This fixes all the checkpatch --file complaints about kernel/ptrace.c
> and also removes an unused #include.  I've verified that there are no
> changes to the compiled code on x86_64.

Please don't bother with that insane "line length" option when using 
"--file". At least not if the "fix" is to just mindlessly split the line. 
That is _never_ a fix.

Changes like these:

> -int ptrace_readdata(struct task_struct *tsk, unsigned long src, char __user *dst, int len)
> +int ptrace_readdata(struct task_struct *tsk, unsigned long src,
> +		    char __user *dst, int len)

> -int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long dst, int len)
> +int ptrace_writedata(struct task_struct *tsk, char __user *src,
> +		     unsigned long dst, int len)

>  	case PTRACE_GETEVENTMSG:
> -		ret = put_user(child->ptrace_message, (unsigned long __user *) data);
> +		ret = put_user(child->ptrace_message,
> +			       (unsigned long __user *) data);

just make the code harder to 'grep'.

Yes, at some point you have to split lines, but that point is not 80
columns any more.  The advantage of getting the whole line when grepping
for function names much outweighs the downside of somebody using those
old 80x24 green phosphorous vt52's.

[ The same thing very much goes for complex if-statements etc.  If
  people can't stand the long lines, the primary solution would be to
  turn a complex conditional into a helper inline functions, or to fix
  excessive indentation by splitting up functions.

  In the above case, the last one could perhaps have been handled
  creating a new variable for and moving the cast to the initialiser,
  for example. Is it worth it to avoid a 85-column line? Probably not.

  And some lines just end up long.  I think 100 characters may be a
  more reasonable limit for "too long", but quite frankly, it depends on
  the line.

  So I think 'checkpatch' is pure crap in this area, and I've told
  people so before, and they keep telling me that it has relaxed it's
  idiotic warnings, but that is apparently just a lie. ]

Oh well.  If I actually read perl, I could parse what the hell those
80-character rules are in checkpath.  It already has random "it's ok if
X" stuff.  But it never seems to really have any "oh, but splitting is
worse" logic. 

			Linus