[webkit-dev] coding style and comments

[Peter Kasting]

On Mon, Jan 31, 2011 at 12:47 PM, Maciej Stachowiak <mjs at apple.com> wrote:

> On Jan 31, 2011, at 11:01 AM, Peter Kasting wrote:
> I think people who favor comments tend to produce a lot of exactly this
> kind of comment. Except in some cases its verbose multiline comments that
> exceed the number of actual lines of code. Here's some more files with many
> comments that are better deleted or replaced with refactoring:
>
>
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.h
>
> http://trac.webkit.org/browser/trunk/Source/WebCore/platform/image-decoders/ImageDecoder.cpp
>

Oh good, files I know something about :)

To pick just one specific example:
>
>    // ICOs always begin with a 2-byte 0 followed by a 2-byte 1.
>    // CURs begin with 2-byte 0 followed by 2-byte 2.
>
> if (!memcmp(contents, "\x00\x00\x01\x00", 4) || !memcmp(contents, "\x00\x00\x02\x00", 4))
>
> This would be more readable if the comments were deleted and the memcmps
> were replaced with calls to inline functions named
> hasICOMagicNumber/hasCURMagicNumber or the like.
>

Or just omitted altogether.  I don't think the comments here add much.
 (They predate my involvement; they actually come from hyatt in 2006, and in
fairness to him, this code at that time was very much "throw in something
quick that works to get a Windows port up and running".)

Your choice of files is interesting otherwise though.  Aside from those
2006-era comments in create(), there are only three other comments in
ImageDecoder.cpp, two of which seem worth having to me.  In ImageDecoder.h,
many function declarations are commented, most to explain ownership details,
caution users of functionality that needs to match in multiple places, or
give more context to why callers would use the function.  There are
definitely some comments lying around here that aren't terribly useful, but
the majority of these have been helpful when tracing through various
different image decoding bugs, especially memory errors triggered by our
heuristic that throws away image data for large images sometimes, or when
refreshing my memory on what this code does when I come back to it after a
year of doing something else.

So I don't agree that "many" comments here (if "many" means "the majority")
are unhelpful.  I guess, then, we do disagree about what types of comments
are useful.

I would go further than that. Even if a comment is valuable, the reviewer
> (and the patch author) should think about whether what it says could be
> expressed in source code instead, with some refactoring. Comments should be
> a last resort for making code clear. I do agree that they have their place,
> but I think that place is fairly rare.
>

I think we are probably going to remain in disagreement, then.  In my
opinion, this is fine if you already know the code in question in detail,
but I agree with Simon that this level of strictness is a barrier to
learning new code and contributes to a "privileged few" sort of view of code
ownership.

PK
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20110131/f125e0a9/attachment.html>