[U-Boot] Policy for checkpatch usage?

[U-Boot] Policy for checkpatch usage?

[Detlev Zundel]

Hi,

currently our CodingStyle[1] has this to say about checkpatch.pl:

  Make sure to run the checkpatch.pl script from the Linux source tree
  to check your patches. Note that this should be done before posting on
  the mailing list!

Now this is not very clear as to what should be done with regard to the
results of such a checkpatch run.  I think we all agree that warnings
tailored to the linux kernel like this can be ignored:

  WARNING: consider using kstrto* in preference to simple_strtol
  #215: FILE: net/tftp.c:619:
  +		TftpRemotePort = simple_strtol(ep, NULL, 10);

[U-Boot] Policy for checkpatch usage?

[Graeme Russ]

On Wednesday, April 20, 2011, Detlev Zundel <dzu@denx.de> wrote:
> Hi,
>

>
> As a base for discussion, what about this:
>
>  ?Use common sense in interpreting the results of checkpatch. Warnings
>  ?that clearly only make sense in the Linux kernel can be ignored. ?Also
>  ?warnings produced for _context lines_ rather than actual changes can
>  ?also be ignored.

One man's common sense is another's idiocy

I vote for a zero warnings, zero errors U-Boot specific checkpatch

Regards,

Graeme

[U-Boot] Policy for checkpatch usage?

[Graeme Russ]

On Wednesday, April 20, 2011, Graeme Russ <graeme.russ@gmail.com> wrote:
> On Wednesday, April 20, 2011, Detlev Zundel <dzu@denx.de> wrote:
>> Hi,
>>
>
>>
>> As a base for discussion, what about this:
>>
>> ??Use common sense in interpreting the results of checkpatch. Warnings
>> ??that clearly only make sense in the Linux kernel can be ignored. ?Also
>> ??warnings produced for _context lines_ rather than actual changes can
>> ??also be ignored.
>
> One man's common sense is another's idiocy
>
> I vote for a zero warnings, zero errors U-Boot specific checkpatch
>

I also think that all patches should be submitted with a checkpatch
summary with an explaination for any errors or warnings - this will at
least save a little effort for the maintainers and reduce the number of
patches bounced only to have the checkpatch problems argued away
by the author anyway

Regards,

Graeme

[U-Boot] Policy for checkpatch usage?

[Detlev Zundel]

Hi Graeme,

> On Wednesday, April 20, 2011, Graeme Russ <graeme.russ@gmail.com> wrote:
>> On Wednesday, April 20, 2011, Detlev Zundel <dzu@denx.de> wrote:
>>> Hi,
>>>
>>
>>>
>>> As a base for discussion, what about this:
>>>
>>> ??Use common sense in interpreting the results of checkpatch. Warnings
>>> ??that clearly only make sense in the Linux kernel can be ignored. ?Also
>>> ??warnings produced for _context lines_ rather than actual changes can
>>> ??also be ignored.
>>
>> One man's common sense is another's idiocy
>>
>> I vote for a zero warnings, zero errors U-Boot specific checkpatch
>>
>
> I also think that all patches should be submitted with a checkpatch
> summary with an explaination for any errors or warnings - this will at
> least save a little effort for the maintainers and reduce the number of
> patches bounced only to have the checkpatch problems argued away
> by the author anyway

When we accept 0 errors and 0 warnings only, then we will always see the
same text :)

As long as we are not there, I do agree but then we should come up with
a recipe on how to automate this.  I looked into git format-patch but it
does not seem to have such an option.  Does anyone have a clever
one-liner for this?

Cheers
  Detlev

-- 
Math and Alcohol don't mix, so...
PLEASE DON'T DRINK AND DERIVE

[Motto of the society: Mathematicians Against Drunk Deriving]
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] Policy for checkpatch usage?

[Detlev Zundel]

Hi Graeme,

> On Wednesday, April 20, 2011, Detlev Zundel <dzu@denx.de> wrote:
>> Hi,
>>
>
>>
>> As a base for discussion, what about this:
>>
>>  ?Use common sense in interpreting the results of checkpatch. Warnings
>>  ?that clearly only make sense in the Linux kernel can be ignored. ?Also
>>  ?warnings produced for _context lines_ rather than actual changes can
>>  ?also be ignored.
>
> One man's common sense is another's idiocy
>
> I vote for a zero warnings, zero errors U-Boot specific checkpatch

Forking checkpatch means that someone has to invest work to maintain it.
Judging from the linux repo there are quite some changes in every
iteration of the kernel, so this will be work for us also:

[dzu at pollux linux (master)]$ git rev-list v2.6.35..v2.6.36 scripts/checkpatch.pl | wc -l
7
[dzu at pollux linux (master)]$ git rev-list v2.6.36..v2.6.37 scripts/checkpatch.pl | wc -l
19
[dzu at pollux linux (master)]$ git rev-list v2.6.37..v2.6.38 scripts/checkpatch.pl | wc -l
4

Do we want to invest this work?  Do you volunteer? ;)

Can't we disable individual messages not relevant for us?  Like
"-Wno-kstrto" or such things?

Cheers
  Detlev

-- 
"The number you have dialed is imaginary. Please rotate your phone 90
degrees and try again."
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] Policy for checkpatch usage?

[Scott Wood]

On Wed, 20 Apr 2011 20:15:40 +1000
Graeme Russ <graeme.russ@gmail.com> wrote:

> On Wednesday, April 20, 2011, Detlev Zundel <dzu@denx.de> wrote:
> > Hi,
> >
> 
> >
> > As a base for discussion, what about this:
> >
> >  ?Use common sense in interpreting the results of checkpatch. Warnings
> >  ?that clearly only make sense in the Linux kernel can be ignored. ?Also
> >  ?warnings produced for _context lines_ rather than actual changes can
> >  ?also be ignored.
> 
> One man's common sense is another's idiocy
> 
> I vote for a zero warnings, zero errors U-Boot specific checkpatch

I vote for "checkpatch is a tool that can help you find some style problems,
but is imperfect, and the things it complains about are of varying
importance".  If you insist on zero warnings, what's the difference between
a warning and an error?  And will there then be a U-Boot-specific coding
style document to match?  Will anyone that wants to submit a patch that
checkpatch erroneously complains about have to first submit a patch for
checkpatch (first learning Perl if need be)?

There's a lot more "common sense" that needs to be applied when writing
software than where to stick what kind and amount of whitespace.
Guidelines are good -- zero-tolerance obedience to a script, not so much.

-Scott

[U-Boot] Policy for checkpatch usage?

[Graeme Russ]

On Thu, Apr 21, 2011 at 2:51 AM, Scott Wood <scottwood@freescale.com> wrote:
> On Wed, 20 Apr 2011 20:15:40 +1000
> Graeme Russ <graeme.russ@gmail.com> wrote:
>
>> On Wednesday, April 20, 2011, Detlev Zundel <dzu@denx.de> wrote:
>> > Hi,
>> >
>>
>> >
>> > As a base for discussion, what about this:
>> >
>> > ??Use common sense in interpreting the results of checkpatch. Warnings
>> > ??that clearly only make sense in the Linux kernel can be ignored. ?Also
>> > ??warnings produced for _context lines_ rather than actual changes can
>> > ??also be ignored.
>>
>> One man's common sense is another's idiocy
>>
>> I vote for a zero warnings, zero errors U-Boot specific checkpatch
>
> I vote for "checkpatch is a tool that can help you find some style problems,
> but is imperfect, and the things it complains about are of varying
> importance". ?If you insist on zero warnings, what's the difference between
> a warning and an error? ?And will there then be a U-Boot-specific coding
> style document to match? ?Will anyone that wants to submit a patch that
> checkpatch erroneously complains about have to first submit a patch for
> checkpatch (first learning Perl if need be)?
>
> There's a lot more "common sense" that needs to be applied when writing
> software than where to stick what kind and amount of whitespace.
> Guidelines are good -- zero-tolerance obedience to a script, not so much.
>

Point taken.

What about my other suggestion - A checkpatch summary with an expalation
for any warnings or errors? See for example my heads-up for checkpatch
warnings - http://lists.denx.de/pipermail/u-boot/2011-April/090144.html

Regards,

Graeme

[U-Boot] Policy for checkpatch usage?

[Wolfgang Denk]

Dear Graeme Russ,

In message <BANLkTimdJkETWEOrJFuxq4hN8gG+DvY=-A@mail.gmail.com> you wrote:
>
> What about my other suggestion - A checkpatch summary with an expalation
> for any warnings or errors? See for example my heads-up for checkpatch
> warnings - http://lists.denx.de/pipermail/u-boot/2011-April/090144.html

For issues like this one should do what the last lines of the
checkpatch output say: "If any of these errors are false positives
report them to the maintainer, see CHECKPATCH in MAINTAINERS."


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Quantum Mechanics is God's version of "Trust me."

[U-Boot] Policy for checkpatch usage?

[Detlev Zundel]

Hi Wolfgang,

> Dear Graeme Russ,
>
> In message <BANLkTimdJkETWEOrJFuxq4hN8gG+DvY=-A@mail.gmail.com> you wrote:
>>
>> What about my other suggestion - A checkpatch summary with an expalation
>> for any warnings or errors? See for example my heads-up for checkpatch
>> warnings - http://lists.denx.de/pipermail/u-boot/2011-April/090144.html
>
> For issues like this one should do what the last lines of the
> checkpatch output say: "If any of these errors are false positives
> report them to the maintainer, see CHECKPATCH in MAINTAINERS."

But as I showed, we already know that we do have false positives in
U-Boot as we miss some Linux constructs.  We should refrain from taking
this as "false positives" to the checkpatch maintainers.  Either we can
suppress them somehow or maybe we should list them?

Cheers
  Detlev

-- 
Warning: this comic occasionally contains strong language (which may be unsuit-
able for children), unusual humor (which may be unsuitable for adults), and ad-
vanced mathematics (which may be unsuitable for liberal-arts majors). /xkcd.org
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] Policy for checkpatch usage?

[Detlev Zundel]

Hi Scott,

> I vote for "checkpatch is a tool that can help you find some style problems,
> but is imperfect, and the things it complains about are of varying
> importance".  If you insist on zero warnings, what's the difference between
> a warning and an error?  And will there then be a U-Boot-specific coding
> style document to match?  Will anyone that wants to submit a patch that
> checkpatch erroneously complains about have to first submit a patch for
> checkpatch (first learning Perl if need be)?

So you would agree to this text:

  Checkpatch is a tool that can help you find some style problems, but is
  imperfect, and the things it complains about are of varying importance.
  So use common sense in interpreting the results.  Warnings that clearly
  only make sense in the Linux kernel can be ignored.  
  
What about the problem with checkpatch errors in current code, i.e. the
origin of this sentence:

  Also warnings produced for context lines (i.e. existing code) rather
  than actual changes can also be ignored.

Do we want this?

> There's a lot more "common sense" that needs to be applied when writing
> software than where to stick what kind and amount of whitespace.
> Guidelines are good -- zero-tolerance obedience to a script, not so much.

I agree 100%, but I also understand that people want to see some
guideline that they can refer to.  So let's see how good a compromise we
can find.

Thanks
  Detlev

-- 
Some people unfortunately like  jumping up and down about spaces but not code.
[...]   I'd rather read good poetry written in very bad hand writing than bad
poetry written in beautiful handwriting, and I think the same is true of code.
           -- Alan Cox <20090701130018.115ce0ea@lxorguk.ukuu.org.uk>
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] Policy for checkpatch usage?

[Eric Cooper]

On Thu, Apr 21, 2011 at 04:29:17PM +0200, Detlev Zundel wrote:
> What about the problem with checkpatch errors in current code, i.e. the
> origin of this sentence:
> 
>   Also warnings produced for context lines (i.e. existing code) rather
>   than actual changes can also be ignored.

How about replacing it with this:

    If you encounter warnings for existing code, not modified by your
    patch, consider submitting a separate, cosmetic-only patch --
    clearly described as such -- that *precedes* your substantive
    patch.

-- 
Eric Cooper             e c c @ c m u . e d u

[U-Boot] Policy for checkpatch usage?

[Fabian Cenedese]

At 10:49 21.04.2011 -0400, Eric Cooper wrote:
>On Thu, Apr 21, 2011 at 04:29:17PM +0200, Detlev Zundel wrote:
>> What about the problem with checkpatch errors in current code, i.e. the
>> origin of this sentence:
>> 
>>   Also warnings produced for context lines (i.e. existing code) rather
>>   than actual changes can also be ignored.
>
>How about replacing it with this:
>
>    If you encounter warnings for existing code, not modified by your
>    patch, consider submitting a separate, cosmetic-only patch --
>    clearly described as such -- that *precedes* your substantive
>    patch.

Is that even possible? The cosmetic patch itself will be surrounded
by context lines which may fire up a warning. So these lines need
to be changed as well to satisy checkpatch. But this new patch
will again include several context lines... until you have to fix up the
whole file. Or did I misunderstand?

bye  Fabi

[U-Boot] Policy for checkpatch usage?

[Eric Cooper]

On Thu, Apr 21, 2011 at 04:56:36PM +0200, Fabian Cenedese wrote:
> Is that even possible? The cosmetic patch itself will be surrounded
> by context lines which may fire up a warning. So these lines need
> to be changed as well to satisy checkpatch. But this new patch
> will again include several context lines... until you have to fix up the
> whole file. Or did I misunderstand?

What's wrong with (cosmetically) fixing all the files that a patch
touches?  That way the project gets incremental cleanup of the code
base as it evolves.

(It would be easy to automate a check for whitespace-only patches to
ease the job of the custodians.  Line-breaking and other style changes
might still require eyeballing.)

-- 
Eric Cooper             e c c @ c m u . e d u

[U-Boot] Policy for checkpatch usage?

[Detlev Zundel]

Hi Eric,

> On Thu, Apr 21, 2011 at 04:56:36PM +0200, Fabian Cenedese wrote:
>> Is that even possible? The cosmetic patch itself will be surrounded
>> by context lines which may fire up a warning. So these lines need
>> to be changed as well to satisy checkpatch. But this new patch
>> will again include several context lines... until you have to fix up the
>> whole file. Or did I misunderstand?
>
> What's wrong with (cosmetically) fixing all the files that a patch
> touches?  That way the project gets incremental cleanup of the code
> base as it evolves.

Of course such a cleanup would be nice indeed, but I assume that many
people do not want to go this extra mile - alas I do not want to require
it.

> (It would be easy to automate a check for whitespace-only patches to
> ease the job of the custodians.  Line-breaking and other style changes
> might still require eyeballing.)

Like this?

-------------- next part --------------
A non-text attachment was scrubbed...
Name: is-whitespace-patch.sh
Type: text/x-sh
Size: 398 bytes
Desc: not available
Url : http://lists.denx.de/pipermail/u-boot/attachments/20110421/62542f99/attachment.sh 
-------------- next part --------------

Cheers
  Detlev

-- 
Question    : If you were redesigning UNIX, what would you do differently?
Ken Thompson: I'd spell creat with an e.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] Policy for checkpatch usage?

[Detlev Zundel]

Hi Fabi,

> At 10:49 21.04.2011 -0400, Eric Cooper wrote:
>>On Thu, Apr 21, 2011 at 04:29:17PM +0200, Detlev Zundel wrote:
>>> What about the problem with checkpatch errors in current code, i.e. the
>>> origin of this sentence:
>>> 
>>>   Also warnings produced for context lines (i.e. existing code) rather
>>>   than actual changes can also be ignored.
>>
>>How about replacing it with this:
>>
>>    If you encounter warnings for existing code, not modified by your
>>    patch, consider submitting a separate, cosmetic-only patch --
>>    clearly described as such -- that *precedes* your substantive
>>    patch.
>
> Is that even possible? The cosmetic patch itself will be surrounded
> by context lines which may fire up a warning. So these lines need
> to be changed as well to satisy checkpatch. But this new patch
> will again include several context lines... until you have to fix up the
> whole file. Or did I misunderstand?

It may become an iterative process.  Fortunately our source files are
finite, so this process has a fixpoint ;)

Cheers
  Detlev

-- 
debian is a prototype for a future version of emacs.
                         -- Thien-Thi Nguyen in <7eekubiffq.fsf@ada2.unipv.it>
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] Policy for checkpatch usage?

[Detlev Zundel]

Hi Eric,

> On Thu, Apr 21, 2011 at 04:29:17PM +0200, Detlev Zundel wrote:
>> What about the problem with checkpatch errors in current code, i.e. the
>> origin of this sentence:
>> 
>>   Also warnings produced for context lines (i.e. existing code) rather
>>   than actual changes can also be ignored.
>
> How about replacing it with this:

Thanks for the input.  Current base for discussion:

  Checkpatch is a tool that can help you find some style problems, but
  is imperfect, and the things it complains about are of varying
  importance.  So use common sense in interpreting the results.

  Warnings that clearly only make sense in the Linux kernel can be
  ignored.  This includes "Use #include <linux/$file> instead of
  <asm/$file>" for example.
  
  If you encounter warnings for existing code, not modified by your
  patch, consider submitting a separate, cosmetic-only patch -- clearly
  described as such -- that *precedes* your substantive patch.

Anyone unhappy with that?  Will this help newcomers?

Happy easter everyone, and remember around easter some people like to
hide eggs, _not_ bugs ;)

  Detlev

-- 
Whenever you find yourself on the side of the majority it is
time to pause and reflect.
                                                -- Mark Twain
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] Policy for checkpatch usage?

[Andreas Pretzsch]

Am Donnerstag, den 21.04.2011, 17:46 +0200 schrieb Detlev Zundel:
> Thanks for the input.  Current base for discussion:
> 
>   Checkpatch is a tool that can help you find some style problems, but
>   is imperfect, and the things it complains about are of varying
>   importance.  So use common sense in interpreting the results.
> 
>   Warnings that clearly only make sense in the Linux kernel can be
>   ignored.  This includes "Use #include <linux/$file> instead of
>   <asm/$file>" for example.
>   
>   If you encounter warnings for existing code, not modified by your
>   patch, consider submitting a separate, cosmetic-only patch -- clearly
>   described as such -- that *precedes* your substantive patch.

For minor modifications (e.g. changed arguments of a function call),
adhere to the present codingstyle of the module. Relating checkpatch
warnings can be ignored in this case. A respective note in the commit or
cover letter why they are ignored is desired.


> Anyone unhappy with that?  Will this help newcomers?

Sounds sensible to me. The extra paragraph above is to circumvent
intermixing codingstyle inside a file. For example in common/main.c,
there are "function (arg, arg)" all around. Not checkpatch compliant
(and ugly, IMHO). Now when I modify one of this calls, e.g add an
argument, I'd have to change this. Which leads to a mixture of styles in
the file, which is even worse than than adhering to the "broken" style.

In general, I'd suggest a - maybe informal - policy that while
codingstyle is important, there are more relevant aspects like elegant
code, sensible interfaces, proper use of static and const and so on.
I'm sure any developer is willing to debate about these issues, but most
volunteers (esp. company or freelance coders) will resign after the 3rd
round of whitespace ping-pong...

Not sure how to phrase that without contradicting the basic idea, maybe
with an additional paragraph like:
"New modules have to be as checkpatch compliant as possible. Violations
have to be justified in the commit or cover letter."


-- 

carpe noctem engineering
Ingenieurbuero fuer Hard- & Software-Entwicklung Andreas Pretzsch
Dipl.-Ing. (FH) Andreas Pretzsch          Tel. +49-(0)731-5521572
Hahnengasse 3                             Fax: +49-(0)731-5521573
89073 Ulm, Germany                        email: apr at cn-eng.de

[U-Boot] Policy for checkpatch usage?

[Detlev Zundel]

Hi Andreas,

> Am Donnerstag, den 21.04.2011, 17:46 +0200 schrieb Detlev Zundel:
>> Thanks for the input.  Current base for discussion:
>> 
>>   Checkpatch is a tool that can help you find some style problems, but
>>   is imperfect, and the things it complains about are of varying
>>   importance.  So use common sense in interpreting the results.
>> 
>>   Warnings that clearly only make sense in the Linux kernel can be
>>   ignored.  This includes "Use #include <linux/$file> instead of
>>   <asm/$file>" for example.
>>   
>>   If you encounter warnings for existing code, not modified by your
>>   patch, consider submitting a separate, cosmetic-only patch -- clearly
>>   described as such -- that *precedes* your substantive patch.
>
> For minor modifications (e.g. changed arguments of a function call),
> adhere to the present codingstyle of the module. Relating checkpatch
> warnings can be ignored in this case. A respective note in the commit or
> cover letter why they are ignored is desired.
>
>
>> Anyone unhappy with that?  Will this help newcomers?
>
> Sounds sensible to me. The extra paragraph above is to circumvent
> intermixing codingstyle inside a file. For example in common/main.c,
> there are "function (arg, arg)" all around. Not checkpatch compliant
> (and ugly, IMHO). Now when I modify one of this calls, e.g add an
> argument, I'd have to change this. Which leads to a mixture of styles in
> the file, which is even worse than than adhering to the "broken" style.

Thanks, I've updated the wiki page accordingly[1].

> In general, I'd suggest a - maybe informal - policy that while
> codingstyle is important, there are more relevant aspects like elegant
> code, sensible interfaces, proper use of static and const and so on.
> I'm sure any developer is willing to debate about these issues, but most
> volunteers (esp. company or freelance coders) will resign after the 3rd
> round of whitespace ping-pong...

I fully agree, but on the other hand if we do have a Codingstyle
document _and_ a tool to check for it, there should be no more than one
round and even this one round should only be neccessary if the poster
didn't previously read the Codingstyle document ;)

> Not sure how to phrase that without contradicting the basic idea, maybe
> with an additional paragraph like:
> "New modules have to be as checkpatch compliant as possible. Violations
> have to be justified in the commit or cover letter."

Let's skip this.  My experience is that people want clear and sharp-cut
policies so that they can decide for themselves _before_ posting whether
they adhere to them.  If we cannot achieve such a strictness in our
forumlation, I expect such clauses to generate more discussion than they
solve.

Thanks everyone for the input!
  Detlev

[1] http://www.denx.de/wiki/U-Boot/Patches

-- 
System going down at 1:45 this afternoon for disk crashing.
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de

[U-Boot] Policy for checkpatch usage?

[Scott Wood]

On Thu, 21 Apr 2011 16:29:17 +0200
Detlev Zundel <dzu@denx.de> wrote:

> Hi Scott,
> 
> > I vote for "checkpatch is a tool that can help you find some style problems,
> > but is imperfect, and the things it complains about are of varying
> > importance".  If you insist on zero warnings, what's the difference between
> > a warning and an error?  And will there then be a U-Boot-specific coding
> > style document to match?  Will anyone that wants to submit a patch that
> > checkpatch erroneously complains about have to first submit a patch for
> > checkpatch (first learning Perl if need be)?
> 
> So you would agree to this text:
> 
>   Checkpatch is a tool that can help you find some style problems, but is
>   imperfect, and the things it complains about are of varying importance.
>   So use common sense in interpreting the results.  Warnings that clearly
>   only make sense in the Linux kernel can be ignored.  

Yes.

That said, if someone wants to maintain a U-Boot version, that'd be great.

> What about the problem with checkpatch errors in current code, i.e. the
> origin of this sentence:
> 
>   Also warnings produced for context lines (i.e. existing code) rather
>   than actual changes can also be ignored.
> 
> Do we want this?

That sounds like a bug in checkpatch, which should be reported.

But there's a similar issue of new code that is added in compliance with
the existing style in the file, which is slightly different from what
checkpatch wants.  I'd usually be inclined to stick with what's already in
the file, as long as it's not too ugly, rather than introduce inconsistency
or reformat a large chunk of code to accommodate a small addition.

-Scott

[U-Boot] Policy for checkpatch usage?

[Graeme Russ]

On 22/04/11 02:10, Scott Wood wrote:
> On Thu, 21 Apr 2011 16:29:17 +0200
> Detlev Zundel <dzu@denx.de> wrote:
> 
>> Hi Scott,
>>
>>> I vote for "checkpatch is a tool that can help you find some style problems,
>>> but is imperfect, and the things it complains about are of varying
>>> importance".  If you insist on zero warnings, what's the difference between
>>> a warning and an error?  And will there then be a U-Boot-specific coding
>>> style document to match?  Will anyone that wants to submit a patch that
>>> checkpatch erroneously complains about have to first submit a patch for
>>> checkpatch (first learning Perl if need be)?
>>
>> So you would agree to this text:
>>
>>   Checkpatch is a tool that can help you find some style problems, but is
>>   imperfect, and the things it complains about are of varying importance.
>>   So use common sense in interpreting the results.  Warnings that clearly
>>   only make sense in the Linux kernel can be ignored.  
> 
> Yes.
> 
> That said, if someone wants to maintain a U-Boot version, that'd be great.

So, if someone maintains a U-Boot fork of checkpatch, keeps it up-to-date
with the Linux version, and pushes patches back up to Linux (to keep them
is sync as much as practicable possible) would we agree that that would be
the most favoured solution?

I'm looking at checkpatch now (and its change history) - If I think I can
take it on, I will send out a call for U-Boot specific checkpatch features

Regards,

Graeme

[U-Boot] Policy for checkpatch usage?

[Albert ARIBAUD]

Le 22/04/2011 02:43, Graeme Russ a ?crit :

> So, if someone maintains a U-Boot fork of checkpatch, keeps it up-to-date
> with the Linux version, and pushes patches back up to Linux (to keep them
> is sync as much as practicable possible) would we agree that that would be
> the most favoured solution?

I don't know about 'the most favoured', but I would agree that it would 
be a good way to implement a "zero error, zero warning" policy that 
actually makes sense, because we'll be the ones who decide what causes 
an error or warning and what does not. We could even serenely make it 
"absolutely zero error, ideally zero warning unless justified" if we can 
control which checks are warnings and which checks are errors.

> I'm looking at checkpatch now (and its change history) - If I think I can
> take it on, I will send out a call for U-Boot specific checkpatch features

Wish you luck -- as I said, I did try once to have a fairly simple 
change put in the Linux checkpatch (make maximum line length a command 
line option), and I got zero answer, both public or private. As 
checkpatch compliance could be attained without this change, I 
eventually gave up, but a reactive 'u-checkpatch.pl' maintainer surely 
will attract my interest -- FWIW. :)

As for 'U-Boot specific features', I would advise to rather consider 
'non-Linux-specific features'. We're having issues with the current 
checkpatch because it is Linux-centric (it either tests for actual Linux 
source-code related features or enforces 'Linux cultural' choices); 
replacing these Linux-specific checks with U-Boot specific checks would 
make the Linux and U-Boot checkpatches diverge.

So my personal Xmas wishlist #1 is to be able to choose the set of 
checks that will be performed and which ones will be errors vs warnings. 
Could be a command line option ('--linux' to apply the set of checks for 
a Linux patch and '--u-boot' for an U-Boot patch) or a configuration 
file, for instance.

> Regards,
>
> Graeme

Amicalement,
-- 
Albert.

[U-Boot] Policy for checkpatch usage?

[Graeme Russ]

On 22/04/11 16:18, Albert ARIBAUD wrote:
> Le 22/04/2011 02:43, Graeme Russ a ?crit :
> 
>> So, if someone maintains a U-Boot fork of checkpatch, keeps it up-to-date
>> with the Linux version, and pushes patches back up to Linux (to keep them
>> is sync as much as practicable possible) would we agree that that would be
>> the most favoured solution?
> 
> I don't know about 'the most favoured', but I would agree that it would be
> a good way to implement a "zero error, zero warning" policy that actually
> makes sense, because we'll be the ones who decide what causes an error or
> warning and what does not. We could even serenely make it "absolutely zero
> error, ideally zero warning unless justified" if we can control which
> checks are warnings and which checks are errors.

Checks which we do not have control over using the Linux checkpatch

> 
>> I'm looking at checkpatch now (and its change history) - If I think I can
>> take it on, I will send out a call for U-Boot specific checkpatch features
> 
> Wish you luck -- as I said, I did try once to have a fairly simple change
> put in the Linux checkpatch (make maximum line length a command line
> option), and I got zero answer, both public or private. As checkpatch
> compliance could be attained without this change, I eventually gave up, but
> a reactive 'u-checkpatch.pl' maintainer surely will attract my interest --
> FWIW. :)

Well, I don't know perl (yet) but the code looks very neat and nicely laid
out and all the checks well documented. Recent activity has not been that
extreme so I don't think keeping a U-Boot version in sync with Linux will
be that hard

> As for 'U-Boot specific features', I would advise to rather consider
> 'non-Linux-specific features'. We're having issues with the current
> checkpatch because it is Linux-centric (it either tests for actual Linux
> source-code related features or enforces 'Linux cultural' choices);
> replacing these Linux-specific checks with U-Boot specific checks would
> make the Linux and U-Boot checkpatches diverge.

I don't think the Linux guys are too concerned about white-space cleanups
in patches which include functional changes, but we are

> So my personal Xmas wishlist #1 is to be able to choose the set of checks
> that will be performed and which ones will be errors vs warnings. Could be
> a command line option ('--linux' to apply the set of checks for a Linux
> patch and '--u-boot' for an U-Boot patch) or a configuration file, for
> instance.

I think wrapping our requirements around a command line option is a good
idea anyway, even if the Linux guys do not accept our changes to
checkpatch. I want to make it so a single option makes any U-Boot fork of
checkpatch behave exactly like the Linux version. That would mean combined
U-Boot/Linux developers only need to worry about a single script

Regards,

Graeme

[U-Boot] Policy for checkpatch usage?

[Wolfgang Denk]

Dear Graeme Russ,

In message <4DB0CF2F.2020701@gmail.com> you wrote:
>
> > That said, if someone wants to maintain a U-Boot version, that'd be great.
> 
> So, if someone maintains a U-Boot fork of checkpatch, keeps it up-to-date
> with the Linux version, and pushes patches back up to Linux (to keep them
> is sync as much as practicable possible) would we agree that that would be
> the most favoured solution?
> 
> I'm looking at checkpatch now (and its change history) - If I think I can
> take it on, I will send out a call for U-Boot specific checkpatch features

I think it wouldbe even better if we could push our changes back into
the "mainline" version of checkpatch, so that the U-Boot specific
behaviour can beenabled by a command line option (checkpatch --uboot ?).

Forking is not so preferrable here, I think.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"We don't care.  We don't have to.  We're the Phone Company."

[U-Boot] Policy for checkpatch usage?

[Graeme Russ]

On 22/04/11 18:54, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <4DB0CF2F.2020701@gmail.com> you wrote:
>>
>>> That said, if someone wants to maintain a U-Boot version, that'd be great.
>>
>> So, if someone maintains a U-Boot fork of checkpatch, keeps it up-to-date
>> with the Linux version, and pushes patches back up to Linux (to keep them
>> is sync as much as practicable possible) would we agree that that would be
>> the most favoured solution?
>>
>> I'm looking at checkpatch now (and its change history) - If I think I can
>> take it on, I will send out a call for U-Boot specific checkpatch features
> 
> I think it wouldbe even better if we could push our changes back into
> the "mainline" version of checkpatch, so that the U-Boot specific
> behaviour can beenabled by a command line option (checkpatch --uboot ?).
> 
> Forking is not so preferrable here, I think.
> 

I agree, but if the Linux guys won't accept patches for U-Boot specific
semantics, what choice do we have?

Regards,

Graeme

[U-Boot] Policy for checkpatch usage?

[Wolfgang Denk]

Dear Graeme Russ,

In message <4DB15DD7.8050404@gmail.com> you wrote:
>
> > I think it wouldbe even better if we could push our changes back into
> > the "mainline" version of checkpatch, so that the U-Boot specific
> > behaviour can beenabled by a command line option (checkpatch --uboot ?).
> > 
> > Forking is not so preferrable here, I think.
> 
> I agree, but if the Linux guys won't accept patches for U-Boot specific
> semantics, what choice do we have?

Did you ask him, and he refused, or is this just a hypothetical
question?

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Where shall I begin, please your Majesty?" he asked. "Begin  at  the
beginning,"  the  King said, gravely, "and go on till you come to the
end: then stop."    - Alice's Adventures in Wonderland, Lewis Carroll

[U-Boot] Policy for checkpatch usage?

[Graeme Russ]

On 22/04/11 22:46, Wolfgang Denk wrote:
> Dear Graeme Russ,
> 
> In message <4DB15DD7.8050404@gmail.com> you wrote:
>>
>>> I think it wouldbe even better if we could push our changes back into
>>> the "mainline" version of checkpatch, so that the U-Boot specific
>>> behaviour can beenabled by a command line option (checkpatch --uboot ?).
>>>
>>> Forking is not so preferrable here, I think.
>>
>> I agree, but if the Linux guys won't accept patches for U-Boot specific
>> semantics, what choice do we have?
> 
> Did you ask him, and he refused, or is this just a hypothetical
> question?
> 

I have now asked on LKML - Let's see what unfolds...

Regards,

Graeme