Re: [webkit-dev] coding style and comments

2011-01-31 Thread Ryosuke Niwa
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

2011-01-31 Thread Konstantin Tokarev


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

2011-01-31 Thread Peter Kasting
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

2011-01-31 Thread Maciej Stachowiak

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

2011-01-31 Thread Peter Kasting
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

2011-01-31 Thread Maciej Stachowiak

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

2011-01-31 Thread Peter Kasting
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

2011-01-31 Thread Aaron Boodman
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

2011-01-31 Thread Maciej Stachowiak

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

2011-01-30 Thread David Kilzer
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

2011-01-30 Thread Adam Barth
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

2011-01-30 Thread Adam Barth
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

2011-01-30 Thread Maciej Stachowiak

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

2011-01-30 Thread Ryan Leavengood
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

2011-01-30 Thread Maciej Stachowiak

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

2011-01-30 Thread Maciej Stachowiak

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

2011-01-28 Thread Peter Kasting
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

2011-01-28 Thread Alexey Proskuryakov

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

2011-01-28 Thread Peter Kasting
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

2011-01-28 Thread Maciej Stachowiak

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


Re: [webkit-dev] coding style and comments

2011-01-27 Thread Eric Seidel
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

2011-01-27 Thread Darin Adler
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

2011-01-27 Thread Ryosuke Niwa
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

2011-01-27 Thread Simon Fraser
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

2011-01-27 Thread Dirk Pranke
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

2011-01-27 Thread Dirk Pranke
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

2011-01-27 Thread Ryan Leavengood
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