On Lindent shortcomings and massive style fixing

On Lindent shortcomings and massive style fixing

[Andrey Utkin]

After some iterations of checkpatch.pl, on a new developed driver
(tw5864), now I have the following:

 $ grep 'WARNING\|ERROR' /src/checkpatch.tw5864 | sort | uniq -c
     31 ERROR: do not use C99 // comments
    147 WARNING: Block comments use a trailing */ on a separate line
    144 WARNING: Block comments use * on subsequent lines
    435 WARNING: line over 80 characters

At this point, Lindent was already used, and checkpatch.pl warnings
introduced by Lindent itself were fixed. Usage of "indent
--linux-style" (which behaves differently BTW) doesn't help anymore,
too.

Could anybody please advise how to sort out these issues
automatically, because they look like perfectly solvable in automated
fashion. Of course manual work would result in more niceness, but I am
not eager to go through hundreds of place of code just to fix "over 80
characters" issues now.

First one ("do not use C99 // comments") looks easy with regexps, but
the other are not.

Is there any known improvements or successors for Lindent? Or could we
get indent/Lindent improved if we collect some money for this task?

If anybody wants to look at actual code, here it is:
https://github.com/bluecherrydvr/linux.git , branch tw5864_stable,
drivers/staging/media/tw5864

Current output of "checkpatch.pl -f" for all source files in the
driver is here:
https://gist.github.com/andrey-utkin/12295148475e34ef948b

Thanks in advance.

Re: On Lindent shortcomings and massive style fixing

[Greg KH]

On Mon, Dec 28, 2015 at 04:33:27PM +0200, Andrey Utkin wrote:
> After some iterations of checkpatch.pl, on a new developed driver
> (tw5864), now I have the following:
> 
>  $ grep 'WARNING\|ERROR' /src/checkpatch.tw5864 | sort | uniq -c
>      31 ERROR: do not use C99 // comments
>     147 WARNING: Block comments use a trailing */ on a separate line
>     144 WARNING: Block comments use * on subsequent lines
>     435 WARNING: line over 80 characters
> 
> At this point, Lindent was already used, and checkpatch.pl warnings
> introduced by Lindent itself were fixed. Usage of "indent
> --linux-style" (which behaves differently BTW) doesn't help anymore,
> too.
> 
> Could anybody please advise how to sort out these issues
> automatically, because they look like perfectly solvable in automated
> fashion. Of course manual work would result in more niceness, but I am
> not eager to go through hundreds of place of code just to fix "over 80
> characters" issues now.

Shouldn't take very long to do so, all of the above can be fixed in less
than a day's worth of work manually.  Or you can use indent to fix up
the line length issues, but watch out for the results, sometimes it's
better to refactor the code than to just blindly accept the output of
that tool.

good luck!

greg k-h

Re: On Lindent shortcomings and massive style fixing

[Mauro Carvalho Chehab]

Em Mon, 28 Dec 2015 07:33:32 -0800
Greg KH <greg@kroah.com> escreveu:

> On Mon, Dec 28, 2015 at 04:33:27PM +0200, Andrey Utkin wrote:
> > After some iterations of checkpatch.pl, on a new developed driver
> > (tw5864), now I have the following:
> > 
> >  $ grep 'WARNING\|ERROR' /src/checkpatch.tw5864 | sort | uniq -c
> >      31 ERROR: do not use C99 // comments
> >     147 WARNING: Block comments use a trailing */ on a separate line
> >     144 WARNING: Block comments use * on subsequent lines
> >     435 WARNING: line over 80 characters
> > 
> > At this point, Lindent was already used, and checkpatch.pl warnings
> > introduced by Lindent itself were fixed. Usage of "indent
> > --linux-style" (which behaves differently BTW) doesn't help anymore,
> > too.
> > 
> > Could anybody please advise how to sort out these issues
> > automatically, because they look like perfectly solvable in automated
> > fashion. Of course manual work would result in more niceness, but I am
> > not eager to go through hundreds of place of code just to fix "over 80
> > characters" issues now.
> 
> Shouldn't take very long to do so, all of the above can be fixed in less
> than a day's worth of work manually.  Or you can use indent to fix up
> the line length issues, but watch out for the results, sometimes it's
> better to refactor the code than to just blindly accept the output of
> that tool.

Yeah, on my experience, letting indent to break long lines end do be
a disaster with require more time to manually fix the driver, than using
some editor that shows the 80 cols break (like kate) and fix the lines
manually. IMHO, there are two problems by letting indent breaking long
lines:

1) indent would break strings on printks. This is something that we don't
want to break strings on multiple lines in the Kernel;

2) It doesn't actually solve the problem of having too complex loops,
with is why the 80 columns warning is meant to warn. Worse than that,
if a piece of code is inside more than 4 or 5 indentation levels, the
resulting code of using indent for 80-cols line break is a total crap.

That's said, on a quick look at the driver, it seems that the 80-cols
violations are mostly (if not all) on the comments, like:

	int i_poc_lsb = (frame_seqno_in_gop << 1); /* why multiplied by two? TODO try without multiplication */

and

#define TW5864_UNDEF_REG_0x0224 0x0224	/* Undeclared in spec (or not yet added to tw5864-reg.h) but used */
#define TW5864_UNDEF_REG_0x4014 0x4014	/* Undeclared in spec (or not yet added to tw5864-reg.h) but used */
#define TW5864_UNDEF_REG_0xA800 0xA800	/* Undeclared in spec (or not yet added to tw5864-reg.h) but used */

Btw, the content of tw5864-reg-undefined.h is weird... Why not just
add the stuff there at tw5864-reg.h and remove the comments for all
defines there?

Also, Lindent already did some crappy 80-cols like breaks, like:

static int pci_i2c_multi_read(struct tw5864_dev *dev, u8 devid, u8 devfn, u8 *buf,
		       u32 count)

(count is misaligned with the open parenthesis)

and:
		val =
		    (1 << 24) + ((devid & 0xfe) << 16) + (buf[i * 2 + 0] << 8) +
		    buf[i * 2 + 1];

Regards,
Mauro

Re: On Lindent shortcomings and massive style fixing

[Andrey Utkin]

On Tue, Dec 29, 2015 at 9:32 AM, Mauro Carvalho Chehab
<mchehab@osg.samsung.com> wrote:
> IMHO, there are two problems by letting indent breaking long
> lines:
>
> 1) indent would break strings on printks. This is something that we don't
> want to break strings on multiple lines in the Kernel;

Yeah, GNU indent does its work badly (although I believe it could be
taught to respect long literals), this makes me want to have a better
tool for clean "relayouting" C code.
I believe that'd require at last a proper syntax parser. With such
tool, it would be straightforward to rewrite source code automatically
to please any demanding reviewer of code style, except for issues of
higher level of thought (like naming or nesting limitations).

> 2) It doesn't actually solve the problem of having too complex loops,
> with is why the 80 columns warning is meant to warn. Worse than that,
> if a piece of code is inside more than 4 or 5 indentation levels, the
> resulting code of using indent for 80-cols line break is a total crap.

Then I'd propose to enforce nesting limitation explicitly, because
Documentation/CodingStyle appreciates low nesting, just doesn't give
exact numbers.
It's better this way, because the programmer could pay attention to N
places of excessive nesting and fix it manually, and then carelessly
reformat NNN places of "80 chars" issues automatically, comparing to
reviewing all NNN places, to figure out if there's excessive nesting,
or not.
(CCed checkpatch.pl maintainers.)

> That's said, on a quick look at the driver, it seems that the 80-cols
> violations are mostly (if not all) on the comments, like:
>
>         int i_poc_lsb = (frame_seqno_in_gop << 1); /* why multiplied by two? TODO try without multiplication */
>
> and
>
> #define TW5864_UNDEF_REG_0x0224 0x0224  /* Undeclared in spec (or not yet added to tw5864-reg.h) but used */
> #define TW5864_UNDEF_REG_0x4014 0x4014  /* Undeclared in spec (or not yet added to tw5864-reg.h) but used */
> #define TW5864_UNDEF_REG_0xA800 0xA800  /* Undeclared in spec (or not yet added to tw5864-reg.h) but used */
>
> Btw, the content of tw5864-reg-undefined.h is weird... Why not just
> add the stuff there at tw5864-reg.h and remove the comments for all
> defines there?

tw5864-reg-undefined.h will be edited for sure (maybe dropped), of
course it won't stay as it is now.
It was generated by script during reverse-engineering that bastard
chip from hell.

> Also, Lindent already did some crappy 80-cols like breaks, like:
>
> static int pci_i2c_multi_read(struct tw5864_dev *dev, u8 devid, u8 devfn, u8 *buf,
>                        u32 count)
>
> (count is misaligned with the open parenthesis)

I just added "static " after indenting.
Actually, Documentation/CodingStyle says nothing about alignment of
function declaration tail on open parenthesis.
Also I'd like to mention that I hate such alignment, because it
requires intermixing of tabs and spaces. I am not aware if this is K&R
thing or not. If it is, then please don't kill me.

Thanks for kind replies, gentlemen.

Re: On Lindent shortcomings and massive style fixing

[Julia Lawall]

On Tue, 29 Dec 2015, Andrey Utkin wrote:

> On Tue, Dec 29, 2015 at 9:32 AM, Mauro Carvalho Chehab
> <mchehab@osg.samsung.com> wrote:
> > IMHO, there are two problems by letting indent breaking long
> > lines:
> >
> > 1) indent would break strings on printks. This is something that we don't
> > want to break strings on multiple lines in the Kernel;
>
> Yeah, GNU indent does its work badly (although I believe it could be
> taught to respect long literals), this makes me want to have a better
> tool for clean "relayouting" C code.
> I believe that'd require at last a proper syntax parser. With such
> tool, it would be straightforward to rewrite source code automatically
> to please any demanding reviewer of code style, except for issues of
> higher level of thought (like naming or nesting limitations).
>
> > 2) It doesn't actually solve the problem of having too complex loops,
> > with is why the 80 columns warning is meant to warn. Worse than that,
> > if a piece of code is inside more than 4 or 5 indentation levels, the
> > resulting code of using indent for 80-cols line break is a total crap.
>
> Then I'd propose to enforce nesting limitation explicitly, because
> Documentation/CodingStyle appreciates low nesting, just doesn't give
> exact numbers.
> It's better this way, because the programmer could pay attention to N
> places of excessive nesting and fix it manually, and then carelessly
> reformat NNN places of "80 chars" issues automatically, comparing to
> reviewing all NNN places, to figure out if there's excessive nesting,
> or not.
> (CCed checkpatch.pl maintainers.)

Personally, I prefer to see only 80 characters per line, as long as it
doesn't require contorting the code in some other way.  So perhaps not
everyone would agree that the issue is only the amount of nesting.

julia

> > That's said, on a quick look at the driver, it seems that the 80-cols
> > violations are mostly (if not all) on the comments, like:
> >
> >         int i_poc_lsb = (frame_seqno_in_gop << 1); /* why multiplied by two? TODO try without multiplication */
> >
> > and
> >
> > #define TW5864_UNDEF_REG_0x0224 0x0224  /* Undeclared in spec (or not yet added to tw5864-reg.h) but used */
> > #define TW5864_UNDEF_REG_0x4014 0x4014  /* Undeclared in spec (or not yet added to tw5864-reg.h) but used */
> > #define TW5864_UNDEF_REG_0xA800 0xA800  /* Undeclared in spec (or not yet added to tw5864-reg.h) but used */
> >
> > Btw, the content of tw5864-reg-undefined.h is weird... Why not just
> > add the stuff there at tw5864-reg.h and remove the comments for all
> > defines there?
>
> tw5864-reg-undefined.h will be edited for sure (maybe dropped), of
> course it won't stay as it is now.
> It was generated by script during reverse-engineering that bastard
> chip from hell.
>
> > Also, Lindent already did some crappy 80-cols like breaks, like:
> >
> > static int pci_i2c_multi_read(struct tw5864_dev *dev, u8 devid, u8 devfn, u8 *buf,
> >                        u32 count)
> >
> > (count is misaligned with the open parenthesis)
>
> I just added "static " after indenting.
> Actually, Documentation/CodingStyle says nothing about alignment of
> function declaration tail on open parenthesis.
> Also I'd like to mention that I hate such alignment, because it
> requires intermixing of tabs and spaces. I am not aware if this is K&R
> thing or not. If it is, then please don't kill me.
>
> Thanks for kind replies, gentlemen.
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>