Re: [webkit-dev] coding style and comments
On Sun, Jan 30, 2011 at 4:41 PM, Ryan Leavengood leaveng...@gmail.comwrote: Maybe the solution here is better documentation outside the source code. I hope some of the more experienced WebKit developers can agree that there are parts of the code that are harder for new developers to dig into. Can't agree more. Articles on http://webkit.org/coding/technical-articles.html are great, and I'd love to see more overviews of how various parts of WebKit work. Some high-level docs that are kept updated might be nice. Of course the key here is keeping them updated, and if it is hard to keep source code comments up to date I don't know how much hope there is for outside docs. I don't think it's realistic for us to keep it updated though because it'll be a huge burden to WebKit contributors. But as long as it's dated, I don't think it'll be an issue. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
31.01.2011, 03:41, Ryan Leavengood leaveng...@gmail.com: Maybe the solution here is better documentation outside the source code. I hope some of the more experienced WebKit developers can agree that there are parts of the code that are harder for new developers to dig into. Some high-level docs that are kept updated might be nice. Of course the key here is keeping them updated, and if it is hard to keep source code comments up to date I don't know how much hope there is for outside docs. Of course this applies to any project and is by no means just a flaw in this project. At least if the comments are in the code there is hope that a reviewer would catch when comments get out of date. Generally it's easier to keep up to date docs inside sources, rather than external ones -- Regards, Konstantin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Sun, Jan 30, 2011 at 4:47 PM, Maciej Stachowiak m...@apple.com wrote: Well, I didn't mean to pick on the authors of this file. This is the impression I get from a lot of code that some call well-commented, by which they mean lots of comments. I agree that the comments you pointed out are pretty unhelpful. I tried to emphasize already that silly comments that just restate the next line of code are not at all what I mean by well commented, and that what I am interested in are comments about subtle but crucial details (e.g. complex ownership rules) or comments that sum up a huge swath of source code (e.g. a class-level comment that covers the critical high-level points the class is responsible for). I honestly don't think there is much disagreement about what kinds of comments are unhelpful. I think the disagreement here comes from past experience, where some people have mostly experienced low-quality and out-of-date comments and are justifiably uninterested in repeating that, and others have been helped by high-quality comments in complex codebases and want to see more of that in WebKit. It seems like the best rule of thumb would be that reviewers should look at any added comments and judge whether the comments are really valuable. I don't think we need to (or should) globally frown on comments -- just on bad comments. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Jan 31, 2011, at 11:01 AM, Peter Kasting wrote: On Sun, Jan 30, 2011 at 4:47 PM, Maciej Stachowiak m...@apple.com wrote: Well, I didn't mean to pick on the authors of this file. This is the impression I get from a lot of code that some call well-commented, by which they mean lots of comments. I agree that the comments you pointed out are pretty unhelpful. I tried to emphasize already that silly comments that just restate the next line of code are not at all what I mean by well commented, and that what I am interested in are comments about subtle but crucial details (e.g. complex ownership rules) or comments that sum up a huge swath of source code (e.g. a class-level comment that covers the critical high-level points the class is responsible for). 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 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. I honestly don't think there is much disagreement about what kinds of comments are unhelpful. I think the disagreement here comes from past experience, where some people have mostly experienced low-quality and out-of-date comments and are justifiably uninterested in repeating that, and others have been helped by high-quality comments in complex codebases and want to see more of that in WebKit. Well, I think a comments are good attitude can result in lots of low-quality comments, instead of reserving comments only for cases where they are high in value. It seems like the best rule of thumb would be that reviewers should look at any added comments and judge whether the comments are really valuable. I don't think we need to (or should) globally frown on comments -- just on bad comments. 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. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Mon, Jan 31, 2011 at 12:47 PM, Maciej Stachowiak m...@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 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Jan 31, 2011, at 1:06 PM, Peter Kasting wrote:
On Mon, Jan 31, 2011 at 12:47 PM, Maciej Stachowiak m...@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.)
Ye another option would be to use named constants for the magic strings.
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 do think the majority of comments in there are unhelpful. Here are some
examples from the header that I hope speak for themselves:
// Whether or not the size information has been decoded yet. This
// default implementation just returns true if the size has been set and
// we have not seen a failure. Decoders may want to override this to
// lazily decode enough of the image to get the size.
virtual bool isSizeAvailable()
{
return !m_failed m_sizeAvailable;
}
// Returns the size of the image.
virtual IntSize size() const
{
return m_size;
}
// The total number of frames for the image. Classes that support
// multiple frames will scan the image data for the answer if they need
// to (without necessarily decoding all of the individual frames).
virtual size_t frameCount() { return 1; }
// The number of repetitions to perform for an animation loop.
virtual int repetitionCount() const { return cAnimationNone; }
// Called to obtain the ImageFrame full of decoded data for rendering.
// The decoder plugin will decode as much of the frame as it can before
// handing back the buffer.
virtual ImageFrame* frameBufferAtIndex(size_t) = 0;
// Whether or not the underlying image format even supports alpha
// transparency.
virtual bool supportsAlpha() const { return true; }
It seems to me that none of these comments add info that is worth the code
bloat.
Regards,
Maciej
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
This thread has probably gone the way of all webkit-dev threads on comments or ChangeLog files -- people's opinions vary, it turns into a bikeshed, and nothing really changes about how we code. Repeat in a year. w.r.t. ImageDecoder specifically, as I mentioned before I do agree that there are some comments that are either worthless or partially so, and I'll try and post some cleanup for this header on https://bugs.webkit.org/show_bug.cgi?id=53455 . PK P.S. I agree with you about assertions being better than comments to document pre- (and post-) conditions (where possible). ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Mon, Jan 31, 2011 at 7:18 PM, Peter Kasting pkast...@chromium.org wrote: P.S. I agree with you about assertions being better than comments to document pre- (and post-) conditions (where possible). Me too (where possible). - a ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Jan 31, 2011, at 7:18 PM, Peter Kasting wrote: This thread has probably gone the way of all webkit-dev threads on comments or ChangeLog files -- people's opinions vary, it turns into a bikeshed, and nothing really changes about how we code. Repeat in a year. Well, even though we didn't come to consensus, I hope we benefitted in exposing the silent majority of the list to some strong opinions on code quality, which definitely *is* a key value for the WebKit project, even if we don't have 100% agreement on the means. I, for one, am happy that we have so many people who care passionately about keeping the code clean and readable. w.r.t. ImageDecoder specifically, as I mentioned before I do agree that there are some comments that are either worthless or partially so, and I'll try and post some cleanup for this header on https://bugs.webkit.org/show_bug.cgi?id=53455 . Would be glad to review any code cleanup if you need me to. P.S. I agree with you about assertions being better than comments to document pre- (and post-) conditions (where possible). I find it very often is, even when it initially seems unlikely, and it's awesome when you run the layout tests in a debug build and see your precondition assertion fail. It's like having regression tests for your comments! Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Jan 28, 2011, at 9:36 AM, Peter Kasting wrote: On Thu, Jan 27, 2011 at 4:27 PM, Simon Fraser simon.fra...@apple.com wrote: I think we have a distinct lack of comments that help novices to understand the code. I feel that we almost have a privileged few mentality in some code; if you can't figure it out by reading the code, then you shouldn't be messing with it. Indeed. This sets a high barrier to entry when trying to learn about any nontrivial subsystem. Even when functions are kept brief and well-named, local simplicity does not eliminate global complexity; in fact it can mask it, making the system appear saner than it really is. In this sense, I disagree that self-documenting code is possible on a large scale (even though I am certainly a fan of making the small-scale units concise, clear, etc.). Back when we were writing the initial Chromium port, Brett Wilson and I had to rewrite the Image subsystem three separate times, each time because we realized we'd gotten the ownership semantics wrong and thought we now understood things better. In this case, a simple function that merely allocates an object is deceptively self-documenting, because it's clear what it does in a local sense, but not in the larger picture of how it fits into the rest of the codebase. No one is suggesting that silly comments like for (int i = 0; i 10; ++i) // Count to 10. are a good thing, but I have never had any reviewer objections to adding more detailed comments about subtle bits. At the very least I agree with Simon's suggestion of class-level comments, especially w.r.t. ownership. An interesting case study would be the (still ongoing?) refactoring of the loader code over the past few months (thanks to Adam Barth and others). Is it easier to understand now than before the refactoring started? How many comments were added in the process (FIXMEs notwithstanding)? Dave ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Sun, Jan 30, 2011 at 7:40 AM, David Kilzer ddkil...@webkit.org wrote: An interesting case study would be the (still ongoing?) refactoring of the loader code over the past few months (thanks to Adam Barth and others). Is it easier to understand now than before the refactoring started? How many comments were added in the process (FIXMEs notwithstanding)? We're not really adding that many comments. Mostly we're breaking down monolithic objects like FrameLoader into more understandable bundles of state. For example, NavigationScheduler is a lot more understandable now that it's separate from FrameLoader: http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp?rev=48958 http://trac.webkit.org/browser/trunk/Source/WebCore/loader/NavigationScheduler.cpp In that case, after breaking NavigationScheduler out of FrameLoader, the code got completely re-written to actually use C++ inheritance instead of faking it with structs. A similar thing happened (although perhaps not as successfully) with HistoryController, which also used to be part of FrameLoader. More recently, we've been removing redundant state. For example, http://trac.webkit.org/changeset/76702/trunk/Source/WebCore/loader/FrameLoader.cpp which means deleting a bunch of comments warning folks about the dangers of not keeping various pieces of state synchronized properly. I could be wrong, but I don't think we've added very many comments at all. Personally, I use FIXMEs a lot in new code. For example, in the XSSFilter I'm working on, there are lots of FIXMEs that are notes about things I haven't gotten around to implementing yet (e.g., that the code needs to handle POST but doesn't yet) or ideas for optimizations that would be premature at this point (e.g., an extra memcpy that could be eliminated). Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
As an example of a harmful comment, this comment just wasted 15 minutes of my time: http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.h#L351 The comment was true when written (although confusing because I thought it meant that these functions are virtual so we should avoid calling them and using encoding() directory), but the comment is no longer meaningful because Document::encoding is no longer virtual. IMHO, the maintenance burden of some comments often out-weighs their value. Some comments can be valuable. For example, this comment has been helpful to me in the past: http://trac.webkit.org/browser/trunk/Source/WebCore/loader/FrameLoader.cpp#L2717 It saved me a bunch of time digging through the history of FrameLoader to understand why that code was written the way it was. Adam On Sun, Jan 30, 2011 at 10:27 AM, Adam Barth aba...@webkit.org wrote: On Sun, Jan 30, 2011 at 7:40 AM, David Kilzer ddkil...@webkit.org wrote: An interesting case study would be the (still ongoing?) refactoring of the loader code over the past few months (thanks to Adam Barth and others). Is it easier to understand now than before the refactoring started? How many comments were added in the process (FIXMEs notwithstanding)? We're not really adding that many comments. Mostly we're breaking down monolithic objects like FrameLoader into more understandable bundles of state. For example, NavigationScheduler is a lot more understandable now that it's separate from FrameLoader: http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp?rev=48958 http://trac.webkit.org/browser/trunk/Source/WebCore/loader/NavigationScheduler.cpp In that case, after breaking NavigationScheduler out of FrameLoader, the code got completely re-written to actually use C++ inheritance instead of faking it with structs. A similar thing happened (although perhaps not as successfully) with HistoryController, which also used to be part of FrameLoader. More recently, we've been removing redundant state. For example, http://trac.webkit.org/changeset/76702/trunk/Source/WebCore/loader/FrameLoader.cpp which means deleting a bunch of comments warning folks about the dangers of not keeping various pieces of state synchronized properly. I could be wrong, but I don't think we've added very many comments at all. Personally, I use FIXMEs a lot in new code. For example, in the XSSFilter I'm working on, there are lots of FIXMEs that are notes about things I haven't gotten around to implementing yet (e.g., that the code needs to handle POST but doesn't yet) or ideas for optimizations that would be premature at this point (e.g., an extra memcpy that could be eliminated). Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
I'll go the other way and point out some example of comments that I think are poor. /// // BlobResourceSynchronousLoader http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L72 This two-line block comment is wasteful. It just states that a class declaration is coming. The class does not need a herald to announce it. // We cannot handle the size that is more than maximum integer. http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L101 If it's not sufficiently clear what the code below is doing it, factor it out into a named function. Also, why use the mystery constant 0x7fff and describe it in a comment as maximum integer when there's a perfectly good symbolic MAXINT constant? // Read all the data. http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L110 Classic what not why comment. If it's not clear enough that the two lines below read all the data, refactor to make the code itself clear. /// // BlobResourceHandle http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L130 Another here comes a class declaration! comment. // static http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L133 Repeats what's in the header and could very easily go out of sync. // We need to take a ref. http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L162 This comment doesn't explain either what the code is doing or why. // Do not continue if the request is aborted or an error occurs. http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L194 The very next lines of code are if (m_aborted || m_errorCode) return; The comment doesn't add anything, just makes the code more verbose. // If the blob data is not found, fail now. http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L198 Redundant with immediately following code. // Parse the Range header we care about. http://trac.webkit.org/browser/trunk/WebCore/platform/network/BlobResourceHandle.cpp?rev=69610#L206 This comment is not only redundant, it's taunting me! It implies there is a special reason to care about Range but doesn't tell me what it is. Most of the other comments in this file are along these same lines. They reduce code density without actually adding information. Regards, Maciej On Jan 27, 2011, at 4:50 PM, Dirk Pranke wrote: Two other ways to potentially answer these questions: 1) What are files that you all think *are* well commented? (pointing at files that aren't probably isn't that useful, and is certainly meaner ;). 2) One file over in that same directory, is a filesystem.py file that I wrote (most of). I think it's pretty well commented, by python standards, but I'm curious what others think. I give you permission to publicly bash me ahead of time ;) I will also note, as a general thought, that comments should not be used as documentation where tests can be used instead. Similarly, people often under-document tests and make them very hard to maintain. Lastly, I volunteer to take whatever wisdom is offered up on this thread and aggregate it onto the Wiki ... -- Dirk On Thu, Jan 27, 2011 at 4:38 PM, Dirk Pranke dpra...@chromium.org wrote: I agree with all of the sentiments below, except for possibly one or two cases. Namely, sometimes you write code where the what is not obvious. In that situation, it may not be possible to change the code to make it more obvious, because you are bound by contract. For example, suppose you are implementing an interface with a method called sort(). The fact that you choose a quicksort or a mergesort is a useful one-line comment, and in my opinion is probably better than just wrapping a method called quicksort() and hoping it gets inlined away. A slight variant of this is that you may be learning or reverse-engineering code where the what is not clear (perhaps because it was poorly rewritten). In the longer-term, ideally you'd clean up the code, but in the short term it might be useful to capture the what in comments until you can safely refactor things and delete the comments. For example, ev...@chromium.org commented on IRC that he often checks out files from the tree, adds local comments to the file, to help keep track of what the code does, but then deletes them prior to checking his patches back in. He's a pretty good guy, so I am guessing that his comments would be useful to others as well (a net improvement to the code and an incremental step towards the refactoring described above), and it's a shame that that
Re: [webkit-dev] coding style and comments
On Sunday, January 30, 2011, Maciej Stachowiak m...@apple.com wrote: I'll go the other way and point out some example of comments that I think are poor. This file is terribly commented, I agree, but cherry picking a really bad file isn't the best way to make a point IMO. Maybe the solution here is better documentation outside the source code. I hope some of the more experienced WebKit developers can agree that there are parts of the code that are harder for new developers to dig into. Some high-level docs that are kept updated might be nice. Of course the key here is keeping them updated, and if it is hard to keep source code comments up to date I don't know how much hope there is for outside docs. Of course this applies to any project and is by no means just a flaw in this project. At least if the comments are in the code there is hope that a reviewer would catch when comments get out of date. -- Regards, Ryan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Jan 28, 2011, at 11:34 AM, Maciej Stachowiak wrote: In general, I agree with what Eric and Darin said about this topic. By the way, here are some good resources that align well with the traditional WebKit project philosophy of commenting: My new favorite statement about this topic is, If you can turn a comment into source code, do it. When adding a comment, try to think - why is this code so unclear that it needs a comment? Is there a way to rewrite it so the source speaks for itself? http://www.c2.com/cgi/wiki?TreatCommentsWithSuspicion More on potential downsides of comments, and how refactoring can be an alternative: http://www.c2.com/cgi/wiki?ToNeedComments Here are some good techniques for making code more naturally self-documenting: http://www.c2.com/cgi/wiki?SelfDocumentingCode Here is a criticism specifically of giant block comments: http://www.c2.com/cgi/wiki?MassiveFunctionHeaders And here is a an argument for why not what commenting: http://www.c2.com/cgi/wiki?CommentTheWhy Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Jan 30, 2011, at 4:41 PM, Ryan Leavengood wrote: On Sunday, January 30, 2011, Maciej Stachowiak m...@apple.com wrote: I'll go the other way and point out some example of comments that I think are poor. This file is terribly commented, I agree, but cherry picking a really bad file isn't the best way to make a point IMO. Well, I didn't mean to pick on the authors of this file. This is the impression I get from a lot of code that some call well-commented, by which they mean lots of comments. Maybe the solution here is better documentation outside the source code. I hope some of the more experienced WebKit developers can agree that there are parts of the code that are harder for new developers to dig into. Some high-level docs that are kept updated might be nice. Of course the key here is keeping them updated, and if it is hard to keep source code comments up to date I don't know how much hope there is for outside docs. Of course this applies to any project and is by no means just a flaw in this project. At least if the comments are in the code there is hope that a reviewer would catch when comments get out of date. I agree that high-level docs would be useful. It's hard to make good ones. My favorite two kinds of non-source-code documentation are explanations of common idioms (e.g. http://webkit.org/coding/RefPtr.html) and explanations of how large-scale subsystems relate to each other. These types of documentation are somewhat less likely to go out of date than function-level or even class-level comments. Often, the best place for them is not in the code. Though it might be a good comprehensive guide to WebKit development that collects the more useful documentation we have. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Thu, Jan 27, 2011 at 4:27 PM, Simon Fraser simon.fra...@apple.comwrote: I think we have a distinct lack of comments that help novices to understand the code. I feel that we almost have a privileged few mentality in some code; if you can't figure it out by reading the code, then you shouldn't be messing with it. Indeed. This sets a high barrier to entry when trying to learn about any nontrivial subsystem. Even when functions are kept brief and well-named, local simplicity does not eliminate global complexity; in fact it can mask it, making the system appear saner than it really is. In this sense, I disagree that self-documenting code is possible on a large scale (even though I am certainly a fan of making the small-scale units concise, clear, etc.). Back when we were writing the initial Chromium port, Brett Wilson and I had to rewrite the Image subsystem three separate times, each time because we realized we'd gotten the ownership semantics wrong and thought we now understood things better. In this case, a simple function that merely allocates an object is deceptively self-documenting, because it's clear what it does in a local sense, but not in the larger picture of how it fits into the rest of the codebase. No one is suggesting that silly comments like for (int i = 0; i 10; ++i) // Count to 10. are a good thing, but I have never had any reviewer objections to adding more detailed comments about subtle bits. At the very least I agree with Simon's suggestion of class-level comments, especially w.r.t. ownership. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
28.01.2011, в 9:36, Peter Kasting написал(а): At the very least I agree with Simon's suggestion of class-level comments, especially w.r.t. ownership. I remember this comment http://trac.webkit.org/browser/trunk/WebCore/platform/PlatformString.h?rev=16622#L44 causing trouble. Here is some history if you're interested: https://bugs.webkit.org/show_bug.cgi?id=11555#c5 https://bugs.webkit.org/show_bug.cgi?id=16550 http://trac.webkit.org/changeset/28977/trunk/WebCore/platform/text/PlatformString.h This comment has evolved into comments for crossThreadString() and threadsafeCopy() - and while these are useful, they are not up to date, mentioning a non-existent copy() function. Do you have any specific mechanism in mind for keeping global comments accurate? Personally, I expect that adding such comments will make the barrier for entry higher in practice, as one would have to deal with lies. - WBR, Alexey Proskuryakov ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Fri, Jan 28, 2011 at 10:14 AM, Alexey Proskuryakov a...@webkit.org wrote: Do you have any specific mechanism in mind for keeping global comments accurate? No more than I have for keeping API usage or function implementations correct; that is, if we have comments, they must be as important to reviewers and patch authors as the code is. Personally, I expect that adding such comments will make the barrier for entry higher in practice, as one would have to deal with lies. At least for me, WebKit subsystems have always been much harder to learn than equivalent-complexity Chromium subsystems (in Chromium we strongly encourage comments, and our style guide mandates at least class-level ones). I have heard this sentiment echoed by others but I can't say whether it's a majority viewpoint. No system is perfect, but I have not found out-of-date comments in Chromium to be a big problem, presumably because the culture prods people to keep them up to date. PK ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Jan 27, 2011, at 4:38 PM, Dirk Pranke wrote:
I agree with all of the sentiments below, except for possibly one or two
cases.
Namely, sometimes you write code where the what is not obvious. In
that situation, it may not be possible to change the code to make it
more obvious, because you are bound by contract. For example, suppose
you are implementing an interface with a method called sort(). The
fact that you choose a quicksort or a mergesort is a useful one-line
comment, and in my opinion is probably better than just wrapping a
method called quicksort() and hoping it gets inlined away.
I ran into this exact situation. I wanted to add a sort function that doesn't
copy, only swaps. It happens that heapsort is such an algorithm. I named the
public interface nonCopyingSort, made it call a function named heapSort, and
wrote a comment explaining why heapSort is an appropriate implementation for
nonCopyingSort:
http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/NonCopyingSort.h
templatetypename RandomAccessIterator, typename Predicate
inline void nonCopyingSort(RandomAccessIterator start, RandomAccessIterator
end, Predicate compareLess)
{
// heapsort happens to use only swaps, not copies, but the essential thing
about
// this function is the fact that it does not copy, not the specific
algorithm
heapSort(start, end, compareLess);
}
Inlining works. IMO it's always better to use a well-named function to document
what your code does than a comment. In this case I did it even though I also
had a comment explaining why the function uses heapsort.
A slight variant of this is that you may be learning or
reverse-engineering code where the what is not clear (perhaps
because it was poorly rewritten). In the longer-term, ideally you'd
clean up the code, but in the short term it might be useful to capture
the what in comments until you can safely refactor things and delete
the comments.
I would prefer for people to refactor instead of adding explanatory comments.
In general, I agree with what Eric and Darin said about this topic.
Regards,
Maciej
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] coding style and comments
Hi all, Have there been any threads or blog posts in the past over an appropriate level of comments in the code? A brief search of the Google and the list archives didn't really turn up anything. From what I've seen, code in WebKit is much less commented than most if not all large projects I've worked on. Is this intentional? If so, why? Also, do we want commenting style to be consistent across languages in the project? For example, Python code is usually pretty well commented, but we've checked in some Python files here with almost no comments, with the rationale being that it's WebKit style. To allay any fears, I'm not suggesting that we should have large chunks of boilerplate or anything like that. I'm aware as anybody of the danger of comments being wrong or getting out of sync with the code. -- Dirk ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
My personal preference (and I'd love to hear from other contributors) is that code should ideally be self-documenting. If I ever find myself writing a what does this do comment, I try to re-write the code to be easier to read. Normally that means breaking it into smaller pieces, and using static inline functions with clear names. I try to only write comments which either explain why (historical information, bug links, etc.) or FIXME's explaining what the code should do in the future. -eric On Thu, Jan 27, 2011 at 3:40 PM, Dirk Pranke dpra...@chromium.org wrote: Hi all, Have there been any threads or blog posts in the past over an appropriate level of comments in the code? A brief search of the Google and the list archives didn't really turn up anything. From what I've seen, code in WebKit is much less commented than most if not all large projects I've worked on. Is this intentional? If so, why? Also, do we want commenting style to be consistent across languages in the project? For example, Python code is usually pretty well commented, but we've checked in some Python files here with almost no comments, with the rationale being that it's WebKit style. To allay any fears, I'm not suggesting that we should have large chunks of boilerplate or anything like that. I'm aware as anybody of the danger of comments being wrong or getting out of sync with the code. -- Dirk ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On the WebKit project we like “why” comments, but not “what” comments. Comments that say what the next block of code does usually are not a good idea. Instead by using appropriate names and granularity we want the code itself to say what the comment would have. We also frown on “textbook” style comments. Long block comments that read like a manifesto about what a code or class will do aren’t typical in WebKit. It’s critical that comments contain information that the code does not, rather than repeating information that is already present in the code. And that comments are brief enough and easy enough to read that they are likely to be noticed and updated as the code changes. I don’t think this has much to do with specific programming languages. A good discussion of comments could revolve around some specific code sequences and a discussion of whether a particular comment is suitable. I’m not sure we can get very far in the abstract. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Thu, Jan 27, 2011 at 3:46 PM, Eric Seidel e...@webkit.org wrote: My personal preference (and I'd love to hear from other contributors) is that code should ideally be self-documenting. I strongly agree with this point. One pit-fall of adding comments is that it gives an illusion of the increased readability. And I believe making code self-evident to avoid adding comments has been a good driving force in cleanup / refactoring WebKit's code base. On Thu, Jan 27, 2011 at 3:46 PM, Darin Adler da...@apple.com wrote: We also frown on “textbook” style comments. Long block comments that read like a manifesto about what a code or class will do aren’t typical in WebKit. I agree that WebKit code base doesn't usually have a block comment for a class, function, etc... and new code should follow that convention. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Jan 27, 2011, at 3:55 PM, Ryosuke Niwa wrote: On Thu, Jan 27, 2011 at 3:46 PM, Eric Seidel e...@webkit.org wrote: My personal preference (and I'd love to hear from other contributors) is that code should ideally be self-documenting. I strongly agree with this point. One pit-fall of adding comments is that it gives an illusion of the increased readability. And I believe making code self-evident to avoid adding comments has been a good driving force in cleanup / refactoring WebKit's code base. On Thu, Jan 27, 2011 at 3:46 PM, Darin Adler da...@apple.com wrote: We also frown on “textbook” style comments. Long block comments that read like a manifesto about what a code or class will do aren’t typical in WebKit. I agree that WebKit code base doesn't usually have a block comment for a class, function, etc... and new code should follow that convention. I think we have a distinct lack of comments that help novices to understand the code. I feel that we almost have a privileged few mentality in some code; if you can't figure it out by reading the code, then you shouldn't be messing with it. So I encourage more comments (and use them fairly copiously, for anything non-obvious, in my own code). I'd particularly encourage comments before each class definition that say what the class is for, what its lifetime is, what the ownership model is, and how many are usually instantiated. Simon ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
I agree with all of the sentiments below, except for possibly one or two cases. Namely, sometimes you write code where the what is not obvious. In that situation, it may not be possible to change the code to make it more obvious, because you are bound by contract. For example, suppose you are implementing an interface with a method called sort(). The fact that you choose a quicksort or a mergesort is a useful one-line comment, and in my opinion is probably better than just wrapping a method called quicksort() and hoping it gets inlined away. A slight variant of this is that you may be learning or reverse-engineering code where the what is not clear (perhaps because it was poorly rewritten). In the longer-term, ideally you'd clean up the code, but in the short term it might be useful to capture the what in comments until you can safely refactor things and delete the comments. For example, ev...@chromium.org commented on IRC that he often checks out files from the tree, adds local comments to the file, to help keep track of what the code does, but then deletes them prior to checking his patches back in. He's a pretty good guy, so I am guessing that his comments would be useful to others as well (a net improvement to the code and an incremental step towards the refactoring described above), and it's a shame that that effort is wasted. Two other specific examples, from the Python code I alluded to in my first message: In Tools/Scripts/webkitpy/common/system, there are the fileset.py, directoryfileset.py, and zipfileset.py files. They are otherwise fine but have no comments in them, probably because the contributor felt that comments were not webkit style. However, it took me a non-trivial amount of time -- and a brief conversation with Mihai, who reviewed the code -- to understand the structure and rationale of the code (what is a fileset for, etc.). A couple of file-level or class-level comments would've eliminated that concern and may save others the same time in the future. Secondly, in zipfileset.py, there is an open() method (line 49). What that method does is entirely non-obvious: It extracts a named member of a zip file and saves it into a file named according to the filename parameter. I think this routine is an example of something that would benefit from both either a what comment (or a renaming of the method, but it's not obvious to me what it should be renamed to, perhaps extract_as or extract_and_save_as) and a why comment (because we need to take the -actual files from the layout test archives and rename them to the -expected versions. -- Dirk [ Note that this is not meant to be a mark against that code, which is generally pretty good IMO. I would have similar feedback on virtually any C++ file I could open up, I suspect. This just happened to be the file that prompted the train of thought. ] On Thu, Jan 27, 2011 at 3:46 PM, Darin Adler da...@apple.com wrote: On the WebKit project we like “why” comments, but not “what” comments. Comments that say what the next block of code does usually are not a good idea. Instead by using appropriate names and granularity we want the code itself to say what the comment would have. We also frown on “textbook” style comments. Long block comments that read like a manifesto about what a code or class will do aren’t typical in WebKit. It’s critical that comments contain information that the code does not, rather than repeating information that is already present in the code. And that comments are brief enough and easy enough to read that they are likely to be noticed and updated as the code changes. I don’t think this has much to do with specific programming languages. A good discussion of comments could revolve around some specific code sequences and a discussion of whether a particular comment is suitable. I’m not sure we can get very far in the abstract. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
Two other ways to potentially answer these questions: 1) What are files that you all think *are* well commented? (pointing at files that aren't probably isn't that useful, and is certainly meaner ;). 2) One file over in that same directory, is a filesystem.py file that I wrote (most of). I think it's pretty well commented, by python standards, but I'm curious what others think. I give you permission to publicly bash me ahead of time ;) I will also note, as a general thought, that comments should not be used as documentation where tests can be used instead. Similarly, people often under-document tests and make them very hard to maintain. Lastly, I volunteer to take whatever wisdom is offered up on this thread and aggregate it onto the Wiki ... -- Dirk On Thu, Jan 27, 2011 at 4:38 PM, Dirk Pranke dpra...@chromium.org wrote: I agree with all of the sentiments below, except for possibly one or two cases. Namely, sometimes you write code where the what is not obvious. In that situation, it may not be possible to change the code to make it more obvious, because you are bound by contract. For example, suppose you are implementing an interface with a method called sort(). The fact that you choose a quicksort or a mergesort is a useful one-line comment, and in my opinion is probably better than just wrapping a method called quicksort() and hoping it gets inlined away. A slight variant of this is that you may be learning or reverse-engineering code where the what is not clear (perhaps because it was poorly rewritten). In the longer-term, ideally you'd clean up the code, but in the short term it might be useful to capture the what in comments until you can safely refactor things and delete the comments. For example, ev...@chromium.org commented on IRC that he often checks out files from the tree, adds local comments to the file, to help keep track of what the code does, but then deletes them prior to checking his patches back in. He's a pretty good guy, so I am guessing that his comments would be useful to others as well (a net improvement to the code and an incremental step towards the refactoring described above), and it's a shame that that effort is wasted. Two other specific examples, from the Python code I alluded to in my first message: In Tools/Scripts/webkitpy/common/system, there are the fileset.py, directoryfileset.py, and zipfileset.py files. They are otherwise fine but have no comments in them, probably because the contributor felt that comments were not webkit style. However, it took me a non-trivial amount of time -- and a brief conversation with Mihai, who reviewed the code -- to understand the structure and rationale of the code (what is a fileset for, etc.). A couple of file-level or class-level comments would've eliminated that concern and may save others the same time in the future. Secondly, in zipfileset.py, there is an open() method (line 49). What that method does is entirely non-obvious: It extracts a named member of a zip file and saves it into a file named according to the filename parameter. I think this routine is an example of something that would benefit from both either a what comment (or a renaming of the method, but it's not obvious to me what it should be renamed to, perhaps extract_as or extract_and_save_as) and a why comment (because we need to take the -actual files from the layout test archives and rename them to the -expected versions. -- Dirk [ Note that this is not meant to be a mark against that code, which is generally pretty good IMO. I would have similar feedback on virtually any C++ file I could open up, I suspect. This just happened to be the file that prompted the train of thought. ] On Thu, Jan 27, 2011 at 3:46 PM, Darin Adler da...@apple.com wrote: On the WebKit project we like “why” comments, but not “what” comments. Comments that say what the next block of code does usually are not a good idea. Instead by using appropriate names and granularity we want the code itself to say what the comment would have. We also frown on “textbook” style comments. Long block comments that read like a manifesto about what a code or class will do aren’t typical in WebKit. It’s critical that comments contain information that the code does not, rather than repeating information that is already present in the code. And that comments are brief enough and easy enough to read that they are likely to be noticed and updated as the code changes. I don’t think this has much to do with specific programming languages. A good discussion of comments could revolve around some specific code sequences and a discussion of whether a particular comment is suitable. I’m not sure we can get very far in the abstract. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] coding style and comments
On Thu, Jan 27, 2011 at 7:27 PM, Simon Fraser simon.fra...@apple.com wrote: I think we have a distinct lack of comments that help novices to understand the code. I feel that we almost have a privileged few mentality in some code; if you can't figure it out by reading the code, then you shouldn't be messing with it. As a WebKit porter who has not yet seriously dug into the WebCore code much beyond platform I tend to get the same feeling as Simon. While a web engine is not a trivial piece of code some parts of WebKit are much more obtuse to non-experts and could benefit from a little more commenting. When trying to debug problems in a port it is very easy to get lost and confused when in WebCore code. Now maybe that is just the nature of the beast and it takes time to grok, but surely there could be some small improvements made with a few comments. So I encourage more comments (and use them fairly copiously, for anything non-obvious, in my own code). I'd particularly encourage comments before each class definition that say what the class is for, what its lifetime is, what the ownership model is, and how many are usually instantiated. +1000! When doing my port for Haiku I was obviously mostly dealing with platform classes and I wrote comments in my build file about what I thought each platform file was for (some are not so obvious, I still don't know why there is both a Pasteboard and a Clipboard.) I think it gets much worse deeper inside WebCore. So I think it could really help new contributors (or anyone really) to have some high-level comments for classes just like Simon says above. -- Regards, Ryan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
