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);
>From this it follows, that we _cannot_ require 0 warnings from
checkpatch. Maybe we can require 0 _errors_?
Moreover, it's only recently that I saw checkpatch warning about lines
that are not even changed in a patch but are only in the context, as for
exmaple here:
WARNING: unnecessary whitespace before a quoted newline
#293: FILE: net/net.c:1604:
debug("Got ICMP ECHO REQUEST, return %d bytes
\n",
Now when this is "fixed", the new patch will then contain a cosmetic
change only intermixed with the "real" changes. Personally I think this
is problematic and contrary to the intention of checking a _patch_, but
it's the way it is.
I think we all agree that we should make reviews as easy as possible.
In order for that, we need to separate cosmetic from functional changes.
Otherwise in large patches it is very difficult to find the meat of a
patch. Only recently this exact problem made Andy Fleming resplit a
large commit[2] into functional and cosmetic[3] changes (ironically the
cosmetic changes were added as a followup to my request of fixing
checkpatch problems). As much as this is appreciated, it is clear that
it takes some effort to do. Ideally we should prevent such work right
from the start.
So what exact wording should we use in our CodingStyle to prevent such
problems, or should we even start our own version of checkpatch tailored
to our project?
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.
Thanks
Detlev
[1] http://www.denx.de/wiki/view/U-Boot/CodingStyle
[2] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/97068
[3] http://article.gmane.org/gmane.comp.boot-loaders.u-boot/97129
--
config LGUEST
If unsure, say N. If curious, say M. If masochistic, say Y.
-- linux/drivers/lguest/Kconfig
--
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: d...@denx.de
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
