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