[webkit-dev] Please include function-level comments in change log entries
It appears that lately most WebCore change log entires don’t include any comments on individual functions. An overall description of the change at the top of the change log entry is valuable, but it is no substitute for describing the changes to each function. Good function-level comments are useful both while reviewing a patch and while revisiting existing code. Personally, I find that writing the function-level comments helps me a lot in reviewing my own patches before I post them. Thanks. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Please include function-level comments in change log entries
On 07/06/2012 10:05 AM, Dan Bernstein wrote: It appears that lately most WebCore change log entires don’t include any comments on individual functions. An overall description of the change at the top of the change log entry is valuable, but it is no substitute for describing the changes to each function. Good function-level comments are useful both while reviewing a patch and while revisiting existing code. Personally, I find that writing the function-level comments helps me a lot in reviewing my own patches before I post them. You forget there is a WebKit policy of not writing comments or otherwise documenting the code. Or at least that's what it looks like. :-( -- --Per Bothner per.both...@oracle.com p...@bothner.com http://per.bothner.com/ ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Please include function-level comments in change log entries
On Friday, July 06, 2012 11:34:01 AM Per Bothner wrote: On 07/06/2012 10:05 AM, Dan Bernstein wrote: It appears that lately most WebCore change log entires donât include any comments on individual functions. An overall description of the change at the top of the change log entry is valuable, but it is no substitute for describing the changes to each function. Good function-level comments are useful both while reviewing a patch and while revisiting existing code. Personally, I find that writing the function-level comments helps me a lot in reviewing my own patches before I post them. You forget there is a WebKit policy of not writing comments or otherwise documenting the code. I think he meant function level comments on ChangeLogs, not on the code itself. Or at least that's what it looks like. :-(-BEGIN PGP SIGNATURE- Version: GnuPG v2.0.19 (GNU/Linux) iEYEABECAAYFAk/3MI8ACgkQ+tuOMzntPWmSvwCfZX7mSpblM8xqtVYfMagmkS1V 6U4AoJOVtSW5QtfGk0C10boluUg+jzoe vPjB -END PGP SIGNATURE- ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Please include function-level comments in change log entries
On Fri, Jul 6, 2012 at 11:34 AM, Per Bothner per.both...@oracle.com wrote: On 07/06/2012 10:05 AM, Dan Bernstein wrote: It appears that lately most WebCore change log entires don’t include any comments on individual functions. An overall description of the change at the top of the change log entry is valuable, but it is no substitute for describing the changes to each function. Good function-level comments are useful both while reviewing a patch and while revisiting existing code. Personally, I find that writing the function-level comments helps me a lot in reviewing my own patches before I post them. You forget there is a WebKit policy of not writing comments or otherwise documenting the code. Or at least that's what it looks like. :-( Dan is talking about per-function descriptions in ChangeLog entries, not in the source code. Indeed, we try to avoid adding comments as much as possible since comments tend to get out-of-date very quickly, we don't want to be spending all our time updating comments. Instead, we try to refactor code so that code is self-evident or add assertions to codify the comments. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Please include function-level comments in change log entries
On Fri, Jul 6, 2012 at 1:05 PM, Dan Bernstein m...@apple.com wrote: It appears that lately most WebCore change log entires don’t include any comments on individual functions. An overall description of the change at the top of the change log entry is valuable, but it is no substitute for describing the changes to each function. Good function-level comments are useful both while reviewing a patch and while revisiting existing code. Personally, I find that writing the function-level comments helps me a lot in reviewing my own patches before I post them. I find that the overhead of maintaining comments outside of the top of the changelog is excessive. Every time I change a patch in a non-trivial way during the review process, I must regenerate the changelogs. Copy/paste/edit the description at the top is not unreasonable. Hand-merging my old changelog with the new one for each method becomes an egregious amount of work. And I don't see how it helps review more than a proper description at the top that mentions the methods of interest as appropriate. Thanks. ___ 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] Please include function-level comments in change log entries
I find that the function level comments, while potentially more time consuming to create are more beneficial than the comments at the top. It makes it not only easier to review but for also when going through the history and trying to see what changes were made. About iterations of the changelog during review updating the log at the top or at the function level (once the comments are already in place) should be an equivalent amount of time, at least in my experience. Konrad Sent from my BlackBerry on the Rogers Wireless Network From: Dana Jansens [mailto:dan...@chromium.org] Sent: Friday, July 06, 2012 02:42 PM To: Dan Bernstein m...@apple.com Cc: WebKit Development webkit-dev@lists.webkit.org Subject: Re: [webkit-dev] Please include function-level comments in change log entries On Fri, Jul 6, 2012 at 1:05 PM, Dan Bernstein m...@apple.commailto:m...@apple.com wrote: It appears that lately most WebCore change log entires don’t include any comments on individual functions. An overall description of the change at the top of the change log entry is valuable, but it is no substitute for describing the changes to each function. Good function-level comments are useful both while reviewing a patch and while revisiting existing code. Personally, I find that writing the function-level comments helps me a lot in reviewing my own patches before I post them. I find that the overhead of maintaining comments outside of the top of the changelog is excessive. Every time I change a patch in a non-trivial way during the review process, I must regenerate the changelogs. Copy/paste/edit the description at the top is not unreasonable. Hand-merging my old changelog with the new one for each method becomes an egregious amount of work. And I don't see how it helps review more than a proper description at the top that mentions the methods of interest as appropriate. Thanks. ___ webkit-dev mailing list webkit-dev@lists.webkit.orgmailto:webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev - This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Please include function-level comments in change log entries
On Fri, Jul 6, 2012 at 11:42 AM, Dana Jansens dan...@chromium.org wrote: On Fri, Jul 6, 2012 at 1:05 PM, Dan Bernstein m...@apple.com wrote: It appears that lately most WebCore change log entires don’t include any comments on individual functions. An overall description of the change at the top of the change log entry is valuable, but it is no substitute for describing the changes to each function. Good function-level comments are useful both while reviewing a patch and while revisiting existing code. Personally, I find that writing the function-level comments helps me a lot in reviewing my own patches before I post them. I find that the overhead of maintaining comments outside of the top of the changelog is excessive. Every time I change a patch in a non-trivial way during the review process, I must regenerate the changelogs. Copy/paste/edit the description at the top is not unreasonable. Perhaps we need a better tool here. Re-generating per-function entries while moving the rest of change log entry shouldn't be too hard. Hand-merging my old changelog with the new one for each method becomes an egregious amount of work. And I don't see how it helps review more than a proper description at the top that mentions the methods of interest as appropriate. I find it very useful during my reviews. It's not necessarily useful to add comments to each function. For example, if you're renaming a function A to B, and then there's no point in adding per-function comments for each function that calls B. On the other hand, if you're making a substantial change to some function C, then it's definitely useful to see what has changed in C and why even if that comment repeats, or preferably point to, some of the information in the log description at the top. Also note that your change log might be read by someone trying to figure out why you made a particular change in one function 5 years from now. He or she may not know the context in which the patch was created (because the structure of the code would have changed so much). In such cases, per-function comments are very valuable and save a lot of his or her time. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Please include function-level comments in change log entries
On 07/06/2012 11:41 AM, Ryosuke Niwa wrote: Indeed, we try to avoid adding comments as much as possible since comments tend to get out-of-date very quickly, we don't want to be spending all our time updating comments. Heavens forbid that someone who actually understands the code should have to update the comments once in a while. Better to keep it inscrutable so newbies spend all of *their* time trying to figure it all out. Instead, we try to refactor code so that code is self-evident or add assertions to codify the comments. You're deluding yourself if you think the code (or any code this large and complicated) is or can be self-evident. I find it quite painful to figure out my way through the WebKit code-base, and I'm hardly inexperienced. The biggest annoyance I found is lack of class-level comments. For example what is an Interpreter? How many instances are there in the system? (I.e. is it a singleton class? Is there one per window? One per thread?) What is the relationship to JSGlobalData, JSGlobalObject, RootObject. There are a lot of these classes, and it takes quite a bit of staring at the code to figure it out. Worse, it's hard to remember it all, so if I come back to the codebase after working on something else I have to figure out all out again: I might remember some aspects (like a class starting with JS is probably some kind of JavaScript object), but not a lot of other relationships and properties of the classes. Those of you who work on WebKit all the time might be comfortable with the lack of comments, but I think it's a misguided and unfriendly policy. Of course sometimes I fail to comments classes and functions where I should, but I understand that's a bug, not a feature, -- --Per Bothner per.both...@oracle.com p...@bothner.com http://per.bothner.com/ ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Please include function-level comments in change log entries
Per: You began by thread-jacking Dan's discussion of ChangeLogs, and now appear to be attacking a seasoned contributors polite response with sarcasm and derision. I recommend you soften your words, and try again on another thread. Commenting, or lack there of, has been discussed at great length on previous webkit-dev threads. I'm sure there are many things we could continue to discuss, but I recommend doing so in a friendlier tone, on a separate thread. I would recommend that we close this thread, Dan's original email was simple a Public Service Announcement. Thank you all, and happy hacking! On Fri, Jul 6, 2012 at 12:54 PM, Per Bothner per.both...@oracle.com wrote: On 07/06/2012 11:41 AM, Ryosuke Niwa wrote: Indeed, we try to avoid adding comments as much as possible since comments tend to get out-of-date very quickly, we don't want to be spending all our time updating comments. Heavens forbid that someone who actually understands the code should have to update the comments once in a while. Better to keep it inscrutable so newbies spend all of *their* time trying to figure it all out. Instead, we try to refactor code so that code is self-evident or add assertions to codify the comments. You're deluding yourself if you think the code (or any code this large and complicated) is or can be self-evident. I find it quite painful to figure out my way through the WebKit code-base, and I'm hardly inexperienced. The biggest annoyance I found is lack of class-level comments. For example what is an Interpreter? How many instances are there in the system? (I.e. is it a singleton class? Is there one per window? One per thread?) What is the relationship to JSGlobalData, JSGlobalObject, RootObject. There are a lot of these classes, and it takes quite a bit of staring at the code to figure it out. Worse, it's hard to remember it all, so if I come back to the codebase after working on something else I have to figure out all out again: I might remember some aspects (like a class starting with JS is probably some kind of JavaScript object), but not a lot of other relationships and properties of the classes. Those of you who work on WebKit all the time might be comfortable with the lack of comments, but I think it's a misguided and unfriendly policy. Of course sometimes I fail to comments classes and functions where I should, but I understand that's a bug, not a feature, -- --Per Bothner per.both...@oracle.com p...@bothner.com http://per.bothner.com/ ___ 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] Pointers and self-documenting code
On Fri, Jul 6, 2012 at 1:35 PM, Joe Mason jma...@rim.com wrote:
As has been noted in a recent thread, WebKit strives to make the code
clear and understandable rather than have embedded comments that can become
obsolete. One common practice that I find quite opaque is for classes to
have functions returning pointers which can never return 0, especially when
such functions are often chained.
For example, is this call safe?
int fontSize =
node-document()-frame()-page()-settings()-minimumFontSize();
(Yes, I could call document()-settings() directly, but I want to
illustrate all the different styles of writing accessors.)
No it is not:
Node::document() returns m_document, with an assert documenting that it
never returns 0
Document::frame() has a comment stating that it CAN return 0
Frame::page() CAN return 0 - I think. It just returns m_page with no
comments - is this the same situation as Node::document(), so m_page is
never 0, but nobody bothered to assert? I deduce that m_page can be 0
because the detachFromPage() function which sets m_page to 0 is defined
next in the file and I happened to look down and notice it.
Page::settings() cannot return 0, since it returns m_settings.get() and
m_settings is an OwnPtr, so I know its lifetime matches that of the Page.
So the correct way to write this is:
int fontSize = 0;
Frame* frame = node-document()-frame();
if (frame) {
Page* page = frame-page();
if (page)
fontSize = page-settings()-minimumFontSize();
}
But the only way to know this is to read the code for each accessor! And
forget about memorizing which is which: Document::settings() can return 0,
but Page::settings() can't, so whether settings() needs to be checked or
not depends on the context.
Is there a reason we don't return a reference from functions that are
guaranteed to be non-zero? Then the above would become:
int fontSize =
node-document().frame()-page()-settings().minimumFontSize();
And the error is easy to spot: all the dereferences without checking for 0
are dangerous. And the compiler will enforce this.
That sounds like a good idea. I like it. I'd like to hear opinions of more
senior contributors though.
- Ryosuke
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Pointers and self-documenting code
On Jul 6, 2012, at 1:35 PM, Joe Mason wrote:
As has been noted in a recent thread, WebKit strives to make the code clear
and understandable rather than have embedded comments that can become
obsolete. One common practice that I find quite opaque is for classes to
have functions returning pointers which can never return 0, especially when
such functions are often chained.
For example, is this call safe?
int fontSize =
node-document()-frame()-page()-settings()-minimumFontSize();
(Yes, I could call document()-settings() directly, but I want to illustrate
all the different styles of writing accessors.)
No it is not:
Node::document() returns m_document, with an assert documenting that it never
returns 0
Document::frame() has a comment stating that it CAN return 0
Frame::page() CAN return 0 - I think. It just returns m_page with no
comments - is this the same situation as Node::document(), so m_page is never
0, but nobody bothered to assert? I deduce that m_page can be 0 because the
detachFromPage() function which sets m_page to 0 is defined next in the file
and I happened to look down and notice it.
Page::settings() cannot return 0, since it returns m_settings.get() and
m_settings is an OwnPtr, so I know its lifetime matches that of the Page.
So the correct way to write this is:
int fontSize = 0;
Frame* frame = node-document()-frame();
if (frame) {
Page* page = frame-page();
if (page)
fontSize = page-settings()-minimumFontSize();
}
It's not at all clear that this is correct.
Is it correct for the font size to be zero if the frame doesn't exist? Seems
dubious.
Is it correct for the font size to be zero if the page doesn't exist? Seems
dubious.
Adding null checking only makes sense if you have a good story for what the
code ought to do if the pointer in question is null.
But the only way to know this is to read the code for each accessor! And
forget about memorizing which is which: Document::settings() can return 0,
but Page::settings() can't, so whether settings() needs to be checked or
not depends on the context.
Is there a reason we don't return a reference from functions that are
guaranteed to be non-zero? Then the above would become:
int fontSize = node-document().frame()-page()-settings().minimumFontSize();
And the error is easy to spot: all the dereferences without checking for 0
are dangerous. And the compiler will enforce this.
This is a questionable policy. Often object properties have transient
nullness. They may be null at time T1 and non-null at time T2, and the caller
may know that the property is non-null from context (the value of some other
field, the fact that it's performed some action that forces the field to become
non-null, etc).
Thus statically enforcing the non-nullness of fields is likely to just make the
code more complicated. And it doesn't buy anything.
-F
I find the common use of things like document()-frame() without checking
the return value of document() is a big impediment to understanding the code,
because I'm never sure if I can take it as gospel or if someone has been
sloppy and forgotten to check return values. And in turn it leads to
sloppiness, since it's easy to forget which accessors do require null checks
and write an invalid chain such as the one above.
Joe
-
This transmission (including any attachments) may contain confidential
information, privileged material (including material protected by the
solicitor-client or other applicable privileges), or constitute non-public
information. Any use of this information by anyone other than the intended
recipient is prohibited. If you have received this transmission in error,
please immediately reply to the sender and delete this information from your
system. Use, dissemination, distribution, or reproduction of this
transmission by unintended recipients is not authorized and may be unlawful.
___
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] Please include function-level comments in change log entries
On 07/06/2012 01:02 PM, Eric Seidel wrote: You began by thread-jacking Dan's discussion of ChangeLogs, and now appear to be attacking a seasoned contributors polite response with sarcasm and derision. Sorry - it wasn't meant to be a thread-jacking, but the issue of commenting in WebKit hit a nerve. The response was indeed polite, but I find the policy misguided and frustrating. I recommend you soften your words, and try again on another thread. ... I recommend doing so in a friendlier tone, on a separate thread. While I apologize for the context, I stand by my words. I don't think my words were out of line or particularly harsh. The tone could have been gentler - but too gentle gets ignored. -- --Per Bothner per.both...@oracle.com p...@bothner.com http://per.bothner.com/ ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] class comments should be encouraged
[For the record, in an appropriate thread. No need to respond if you feel this has been adequately discussed in the past. I'm happy to let the thread die.] The biggest WebKit annoyance I have is lack of class-level comments. For example what is an Interpreter? How many instances are there in the system? (I.e. is it a singleton class? Is there one per window? One per thread?) What is the relationship to JSGlobalData, JSGlobalObject, RootObject. There are a lot of these classes, and it takes quite a bit of staring at the code to figure it out. Worse, it's hard to remember it all, so if I come back to the codebase after working on something else I have to figure out all out again: I might remember some aspects (like a class starting with JS is probably some kind of JavaScript object), but not a lot of other relationships and properties of the classes. Documenting what a function/method does is sometimes useful, but what is really important is documenting what (an instance of) a class *is* and (preferably) how it relates to other objects. -- --Per Bothner per.both...@oracle.com p...@bothner.com http://per.bothner.com/ ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Class-level comments in the source code
Starting a new thread, since this has wandered a fair bit off topic (and Eric had a good point that it's kind of a threadjack). On Jul 6, 2012, at 12:54 PM, Per Bothner per.both...@oracle.com wrote: The biggest annoyance I found is lack of class-level comments. For example what is an Interpreter? How many instances are there in the system? (I.e. is it a singleton class? Is there one per window? One per thread?) What is the relationship to JSGlobalData, JSGlobalObject, RootObject. There are a lot of these classes, and it takes quite a bit of staring at the code to figure it out. Worse, it's hard to remember it all, so if I come back to the codebase after working on something else I have to figure out all out again: I might remember some aspects (like a class starting with JS is probably some kind of JavaScript object), but not a lot of other relationships and properties of the classes. My recollection is that in past discussions, most folks were in favor of class-level comments that describe the purpose and nature of the class, particularly if not self-evident from the name. I think we'd likely take patches to take such comments. We also generally like why comments inside functions - comments that explain why something is done that way. What we don't really like is function-level what comments - ones that just restate what the code says. Stuff like this: // increment reference count by one refCount++; Comments like this make the function longer and harder to read, without actually adding explanatory value. In other cases, comments describe what a sequence of steps does where often a well-named function could achieve the same explanatory purpose. Or they describe a precondition or postcondition where often an ASSERT would be better. Class-level comments don't really fall into these categories, and certainly the purpose of some of the objects you mention is not self-evident from the name (as with e.g. AffineTransform). Documenting ownership and lifetime relationships is also useful. We have tended to do that as diagrams separate from the code, since the places where it is most needed tend to be complicated clusters of related objects. Perhaps we could include references to such diagrams in class comments too, if anyone was willing to maintain such diagrams for an extended period. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
As Eric pointed out, this is completely off the topic of the thread now. So
moving to a new thread.
On Fri, Jul 6, 2012 at 12:54 PM, Per Bothner per.both...@oracle.com wrote:
On 07/06/2012 11:41 AM, Ryosuke Niwa wrote:
Indeed, we try to avoid adding comments as much as possible since
comments tend to get out-of-date very quickly, we don't want to be
spending all our time updating comments.
Heavens forbid that someone who actually understands the code should have
to update the comments once in a while. Better to keep it inscrutable
so newbies spend all of *their* time trying to figure it all out.
That's good, right? People who are new to the project should be reading the
code to understand what each class/function does instead of reading the
comment and deluding himself as if he or she understands the code.
Instead, we try to refactor
code so that code is self-evident or add assertions to codify the
comments.
You're deluding yourself if you think the code (or any code this large and
complicated) is or can be self-evident.
That's a pretty strong claim. I find that the vast majority of codebase to
be quite self-evident. Now, it seems like you do work on JSC, and I
understand JS JIT and JS interpreters could be very complicated.
If you think the code in JSC or other parts of WebKit, then please file
bugs to refactor such code to make them more self-explanatory. I hope we
can both agree that code such as:
Node* highestAncestor(Node* node)
{
ASSERT(node);
Node* parent = node;
while (node = node-parentNode())
parent = node;
return parent;
}
doesn't need any comments. We should strive to make all code as
self-evident as this one.
I find it quite painful to figure out my way through the WebKit code-base,
and I'm hardly inexperienced.
I started working on WebKit as a college intern. By the second or the third
months of my internship, I had very little trouble reading the case base.
Of course, things may have been different if I had worked on JSC, line
layout, rendering code, etc... because they tend to be much more
complicated but, nevertheless, your anecdotal evidence doesn't give us a
reason to add more comments.
The biggest annoyance I found is lack of class-level comments.
The lack of class-level comments was also brought up as a significant
contribution barrier the last time we discussed this as well. So perhaps we
need to re-consider adding some class-level comments for some classes.
For example what is an Interpreter?
Interpreter is http://en.wikipedia.org/wiki/Interpreter_(computing), right?
I would be surprised if it weren't.
How many instances are there in the system?
(I.e. is it a singleton class? Is there one per window? One per thread?)
Okay, I had never heard of this class so I opened up
JavaScriptCore.xcodeproj and searched for new Interpreter. JSGlobalData
has it as a member so clearly it's not a singleton class. And there's a
nice comment at the beginning of the class declaration of JSGlobalData that
says WebCore has a one-to-one mapping of threads to JSGlobalData.
What is the relationship to JSGlobalData, JSGlobalObject, RootObject.
I can't tell the distinction between JSGlobalData and JSGlobalObject either
so maybe we should rename these classes. But then I'm not a JSC expert and
there might be a good reason for their seemingly vague names.
There are a lot of these classes, and it takes quite a bit of staring at
the code to figure it out. Worse, it's hard to remember it all, so if I
come back to the codebase after working on something else I have to
figure out all out again: I might remember some aspects (like a class
starting with JS is probably some kind of JavaScript object), but not
a lot of other relationships and properties of the classes.
Those of you who work on WebKit all the time might be comfortable
with the lack of comments, but I think it's a misguided and unfriendly
policy.
I agree that WebKit codebase may not be the most friendly project to
someone who is new to the project.
On the other hand, we don't want to have valuable contributors spend all
their time updating adding comments. And the lack of comments doesn't
seem to be a show stopper for people who frequently contribute and make
significant contributions to the project.
- Ryosuke
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Class-level comments in the source code
On Fri, Jul 6, 2012 at 1:51 PM, Per Bothner per.both...@oracle.com wrote: Documenting what a function/method does is sometimes useful, but what is really important is documenting what (an instance of) a class *is* and (preferably) how it relates to other objects. For some classes, yes. On Fri, Jul 6, 2012 at 2:02 PM, Maciej Stachowiak m...@apple.com wrote: On Jul 6, 2012, at 12:54 PM, Per Bothner per.both...@oracle.com wrote: The biggest annoyance I found is lack of class-level comments. For example what is an Interpreter? How many instances are there in the system? (I.e. is it a singleton class? Is there one per window? One per thread?) What is the relationship to JSGlobalData, JSGlobalObject, RootObject. There are a lot of these classes, and it takes quite a bit of staring at the code to figure it out. Worse, it's hard to remember it all, so if I come back to the codebase after working on something else I have to figure out all out again: I might remember some aspects (like a class starting with JS is probably some kind of JavaScript object), but not a lot of other relationships and properties of the classes. My recollection is that in past discussions, most folks were in favor of class-level comments that describe the purpose and nature of the class, particularly if not self-evident from the name. I think we'd likely take patches to take such comments. We also generally like why comments inside functions - comments that explain why something is done that way. ... Class-level comments don't really fall into these categories, and certainly the purpose of some of the objects you mention is not self-evident from the name (as with e.g. AffineTransform). I'd argue that we should still strive to make class names self-explanatory as much as possible, and use comments as the last resort. I've found that our culture of not adding comments have given me a pressure to think really hard to come up with better class names rather than going with vague names with explanatory comments. In some cases, it made me realize that the particular object relationships I had in my mind was not a good design and made me come up with a new design that involved easier-to-understand classes. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Pointers and self-documenting code
From: Filip Pizlo [fpi...@apple.com] Sent: Friday, July 06, 2012 4:52 PM To: Joe Mason Cc: WebKit Development [webkit-dev@lists.webkit.org] Subject: Re: [webkit-dev] Pointers and self-documenting code It's not at all clear that this is correct. Is it correct for the font size to be zero if the frame doesn't exist? Seems dubious. Is it correct for the font size to be zero if the page doesn't exist? Seems dubious. Adding null checking only makes sense if you have a good story for what the code ought to do if the pointer in question is null. Well, yes, but that's not the point of the example. I thought about adding a comment, or some suitable default, but I figured that would be pedantic. Regardless of whether my example default was a good one, it's clear that dereferencing null and crashing is NOT the correct thing to do. This is a questionable policy. Often object properties have transient nullness. They may be null at time T1 and non-null at time T2, and the caller may know that the property is non-null from context (the value of some other field, the fact that it's performed some action that forces the field to become non-null, etc). In this case the caller should specifically assert that the property is not null, to let the person reading the code know that it's a possibility that's been considered and it's believed to be impossible. Thus statically enforcing the non-nullness of fields is likely to just make the code more complicated. And it doesn't buy anything. It buys a lot! It buys clarity. It makes the code less fragile. It's a lower barrier to entry, because less knowledge is needed to understand the code. In some cases it provides a compile-time correctness guarantee, which is a good thing - not in all cases, but sometimes is better than never. There are three categories of accessor: 1. Those that can never return null 2. Those that can potentially return null, and in circumstances complex enough that developers can't usefully keep track while writing algorithms 3. Those that can potentially return null, but only in circumstances that are well defined and easy to reason about (the transient nullness you mention) For type 1, I argue that we should return references to indicate this statically. I don't see how this makes the code more complicated. For type 2, we need to always do null checks. Anything else would be unsafe. For type 3, we're in the exact situation we are now - the function signature will return a pointer, and the only way to know whether it needs to be checked for null or not is to read the code and reason about its usage. I would argue that we're currently too stingy with null checks for type 3, because it's very hard to tell why it's safe to skip null checks by looking at small parts of the code, and it's better to err on the side of safety. But that's a more subtle discussion, and I don't want to focus on that part. I'm more interested in the low-hanging fruit of type 1 vs type 2 right now. Joe - This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Pointers and self-documenting code
On Fri, Jul 6, 2012 at 2:26 PM, Joe Mason jma...@rim.com wrote: From: Filip Pizlo [fpi...@apple.com] Sent: Friday, July 06, 2012 4:52 PM To: Joe Mason Cc: WebKit Development [webkit-dev@lists.webkit.org] Subject: Re: [webkit-dev] Pointers and self-documenting code It's not at all clear that this is correct. Is it correct for the font size to be zero if the frame doesn't exist? Seems dubious. Is it correct for the font size to be zero if the page doesn't exist? Seems dubious. Adding null checking only makes sense if you have a good story for what the code ought to do if the pointer in question is null. Well, yes, but that's not the point of the example. I thought about adding a comment, or some suitable default, but I figured that would be pedantic. Regardless of whether my example default was a good one, it's clear that dereferencing null and crashing is NOT the correct thing to do. This is a questionable policy. Often object properties have transient nullness. They may be null at time T1 and non-null at time T2, and the caller may know that the property is non-null from context (the value of some other field, the fact that it's performed some action that forces the field to become non-null, etc). In this case the caller should specifically assert that the property is not null, to let the person reading the code know that it's a possibility that's been considered and it's believed to be impossible. Thus statically enforcing the non-nullness of fields is likely to just make the code more complicated. And it doesn't buy anything. It buys a lot! It buys clarity. It makes the code less fragile. It's a lower barrier to entry, because less knowledge is needed to understand the code. In some cases it provides a compile-time correctness guarantee, which is a good thing - not in all cases, but sometimes is better than never. There are three categories of accessor: 1. Those that can never return null 2. Those that can potentially return null, and in circumstances complex enough that developers can't usefully keep track while writing algorithms 3. Those that can potentially return null, but only in circumstances that are well defined and easy to reason about (the transient nullness you mention) For type 1, I argue that we should return references to indicate this statically. I don't see how this makes the code more complicated. This change seems valuable. I've seen a lot of code patches where people did something along the line of: Editor* editor= m_frame-editor(); if (!editor) return; Note that Editor is a member variable of Frame and we just return m_editor in editor(). - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Class-level comments in the source code
On 07/06/2012 02:18 PM, Ryosuke Niwa wrote: I'd argue that we should still strive to make class names self-explanatory as much as possible, and use comments as the last resort. Fair enough. However, it's hard to come up with reasonably short and readable names that are self-explanatory. Yes, a good name is very helpful, and can suggest the usage, but mostly if you already have a lot of context. I've found that our culture of not adding comments have given me a pressure to think really hard to come up with better class names rather than going with vague names with explanatory comments. In some cases, it made me realize that the particular object relationships I had in my mind was not a good design and made me come up with a new design that involved easier-to-understand classes. Pressure to come up with good names isn't incompatible with comments ... -- --Per Bothner per.both...@oracle.com p...@bothner.com http://per.bothner.com/ ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Class-level comments in the source code
On 07/06/2012 02:02 PM, Maciej Stachowiak wrote: [... reasonable stuff I fully agree with - but one question ...] Documenting ownership and lifetime relationships is also useful. We have tended to do that as diagrams separate from the code, since the places where it is most needed tend to be complicated clusters of related objects. Wouldn't such diagrams be harder to keep up-to-date than comments? -- --Per Bothner per.both...@oracle.com p...@bothner.com http://per.bothner.com/ ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] class comments should be encouraged
On 07/06/2012 01:51 PM, Per Bothner wrote: [For the record, in an appropriate thread. No need to respond if you feel this has been adequately discussed in the past. I'm happy to let the thread die.] Let's ignore this thread - we already have 2 other new treads ... -- --Per Bothner per.both...@oracle.com p...@bothner.com http://per.bothner.com/ ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Class-level comments in the source code
On Jul 6, 2012 2:36 PM, Per Bothner per.both...@oracle.com wrote: On 07/06/2012 02:18 PM, Ryosuke Niwa wrote: I've found that our culture of not adding comments have given me a pressure to think really hard to come up with better class names rather than going with vague names with explanatory comments. In some cases, it made me realize that the particular object relationships I had in my mind was not a good design and made me come up with a new design that involved easier-to-understand classes. Pressure to come up with good names isn't incompatible with comments ... It's a problem of incentives. In the world where we write per-class comments for every class, there might be less incentive or simply time for coming up with better names because we can always clarify the intent in comments. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Class-level comments in the source code
It is indeed a problem of incentives. Nobody has the incentive to maintain a class comment when making changes, since comments are not checked by the compiler. Therefore, it's much better to not write comments, and instead find other ways of making the code legible. -F On Jul 6, 2012, at 3:02 PM, Ryosuke Niwa rn...@webkit.org wrote: On Jul 6, 2012 2:36 PM, Per Bothner per.both...@oracle.com wrote: On 07/06/2012 02:18 PM, Ryosuke Niwa wrote: I've found that our culture of not adding comments have given me a pressure to think really hard to come up with better class names rather than going with vague names with explanatory comments. In some cases, it made me realize that the particular object relationships I had in my mind was not a good design and made me come up with a new design that involved easier-to-understand classes. Pressure to come up with good names isn't incompatible with comments ... It's a problem of incentives. In the world where we write per-class comments for every class, there might be less incentive or simply time for coming up with better names because we can always clarify the intent in comments. - Ryosuke ___ 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] Comments in the code (Was Please include function-level comments in change log entries)
On 07/06/2012 02:02 PM, Ryosuke Niwa wrote:
As Eric pointed out, this is completely off the topic of the thread now.
So moving to a new thread.
[Alas we now have 3 new treads ...]
Heavens forbid that someone who actually understands the code should
have
to update the comments once in a while. Better to keep it inscrutable
so newbies spend all of *their* time trying to figure it all out.
That's good, right? People who are new to the project should be reading
the code to understand what each class/function does instead of reading
the comment and deluding himself as if he or she understands the code.
Yes and no. Trying to infer structure and intention from chasing
calls and cross-references in the code is tedious and error-prone.
I may think I understanding the code, but I may miss some critical
dependency and prerequisite - something that would be obvious to
someone who understand the code.
Furthermore, expecting someone new to grok the whole picture before
they can get started is rather intense. And it's not just beginners:
I doubt anyone can have a full accurate mental model of every class and
dependency of WebKit, so some shortcuts help every developer. Good class
and method names are very valuable, but so are well-chosen comments.
You're deluding yourself if you think the code (or any code this
large and
complicated) is or can be self-evident.
That's a pretty strong claim. I find that the vast majority of codebase
to be quite self-evident.
If you (who I gather has quite a bit of WebKit experience) find something
self-evident, does not mean it is is.
Now, it seems like you do work on JSC, and I
understand JS JIT and JS interpreters could be very complicated.
Yep. I certainly don't claim to understand all or even most of it.
If you think the code in JSC or other parts of WebKit, then please file
bugs to refactor such code to make them more self-explanatory. I hope we
can both agree that code such as:
Node* highestAncestor(Node* node)
{
ASSERT(node);
Node* parent = node;
while (node = node-parentNode())
parent = node;
return parent;
}
doesn't need any comments. We should strive to make all code as
self-evident as this one.
I agree - at least I can't think of anything useful to add.
For example what is an Interpreter?
...
How many instances are there in the system?
(I.e. is it a singleton class? Is there one per window? One per
thread?)
Okay, I had never heard of this class so I opened up
JavaScriptCore.xcodeproj and searched for new
Interpreter. JSGlobalData has it as a member so clearly it's not a
singleton class. And there's a nice comment at the beginning of the
class declaration of JSGlobalData that says WebCore has a one-to-one
mapping of threads to JSGlobalData.
Right - we can figure a lot of this stuff out by searching through the
code. But each of these searches may take 5 minutes, and there may be
be dozens of these questions one needs to answer to understand how it
all fits together. Then an hour later you lose track of where you started,
so you need to take notes. So you're tempted to submit a patch so you
didn't have to do this next time you want to figure it out - assuming
you feel such patches are welcome (and you've worked out an acceptable
procedure with your employer for submitting patches ...).
I agree that WebKit codebase may not be the most friendly project to
someone who is new to the project.
On the other hand, we don't want to have valuable contributors spend all
their time updating adding comments. And the lack of comments doesn't
seem to be a show stopper for people who frequently contribute and make
significant contributions to the project.
We all have limited resources, and must prioritize. But I think keeping
useful comments up-to-date shouldn't take a lot of time. If nothing else
it's a useful sanity check: If something changes enough that you need to
change a comment, that suggests it's a good thing to take the time to
think through the implications well enough to write them down.
--
--Per Bothner
per.both...@oracle.com p...@bothner.com http://per.bothner.com/
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Jul 6, 2012 3:16 PM, Per Bothner per.both...@oracle.com wrote: On 07/06/2012 02:02 PM, Ryosuke Niwa wrote: Heavens forbid that someone who actually understands the code should have to update the comments once in a while. Better to keep it inscrutable so newbies spend all of *their* time trying to figure it all out. That's good, right? People who are new to the project should be reading the code to understand what each class/function does instead of reading the comment and deluding himself as if he or she understands the code. Yes and no. Trying to infer structure and intention from chasing calls and cross-references in the code is tedious and error-prone. I may think I understanding the code, but I may miss some critical dependency and prerequisite - something that would be obvious to someone who understand the code. That happens even if we had comments. Furthermore, expecting someone new to grok the whole picture before they can get started is rather intense. And it's not just beginners: I doubt anyone can have a full accurate mental model of every class and dependency of WebKit, so some shortcuts help every developer. I'm not sure. Having such shortcuts may increase the chance of people deluding themselves as if they understand Good class and method names are very valuable, but so are well-chosen comments. I can't agree more. The real question is what well-chosen comments are. You're deluding yourself if you think the code (or any code this large and complicated) is or can be self-evident. That's a pretty strong claim. I find that the vast majority of codebase to be quite self-evident. If you (who I gather has quite a bit of WebKit experience) find something self-evident, does not mean it is is. To be fair, as I said, I was able to read WebKit code without much trouble 2-3 months into my internship. I'm not sympathetic to people who can't read code as well as an undergraduate college student. Also, we need require some kind of prior knowledge of things. For example, InlineBox might be a foreign concept to you but it's plain dead obvious for anyone who has read CSS2.1 specifications. Similarly, there might be some classes that are self-explanatory for someone who knows ES5 well and confusing for someone who is not. Where should we draw a line then? Right - we can figure a lot of this stuff out by searching through the code. But each of these searches may take 5 minutes, and there may be be dozens of these questions one needs to answer to understand how it all fits together. Then an hour later you lose track of where you started, so you need to take notes. That sounds like a necessary process to work on any mid-large size project written in C++. I mean WebKit is a state-of-the-art Web browser engine, not a typical web app or business app used by a handful of people. It's litereally used by billions of people around the globe. I hope you and other new contributor understand the code well before submitting patches. I'm not saying that we should never add comments. But I've got an impression that you're trying to bypass some important step in contributing to any large project: RTFC. We all have limited resources, and must prioritize. But I think keeping useful comments up-to-date shouldn't take a lot of time. It DOES. Even today, I find many comments (even FIXMEs) that are out-of-date. Say you add a some comment to a function A because of some odd behavior of another function B, which A calls. Someone awesome later gets rid of the odd behavior in the function B by modifying yet another function C, which B calls. Now, how is this awesome person supposed to know to update the comment in A? If nothing else it's a useful sanity check: If something changes enough that you need to change a comment, that suggests it's a good thing to take the time to think through the implications well enough to write them down. We do that in change logs. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Fri, Jul 6, 2012 at 3:49 PM, Ryosuke Niwa rn...@webkit.org wrote: It DOES. Even today, I find many comments (even FIXMEs) that are out-of-date. Say you add a some comment to a function A because of some odd behavior of another function B, which A calls. Someone awesome later gets rid of the odd behavior in the function B by modifying yet another function C, which B calls. Now, how is this awesome person supposed to know to update the comment in A? Although I mostly agree with Ryosuke on comments... Since this subject comes from time to time, shouldn't we try having class comments as an experiment the help newcomers? If someone can come with good description for a class, review it. If class comments become harmful, they are easy to remove. I doubt many classes will benefit from such comments anyway. Benjamin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Class-level comments in the source code
On Jul 6, 2012 3:04 PM, Filip Pizlo fpi...@apple.com wrote: It is indeed a problem of incentives. Nobody has the incentive to maintain a class comment when making changes, since comments are not checked by the compiler. Therefore, it's much better to not write comments, and instead find other ways of making the code legible. I think explaning the relationship between classes for ones that are not self-explanatory will be helpful and will probably incur very little cost compared to comments in functions. But I do agree that keeping them up-to-date is very challenging. This is why I usually prefer keeping such comments in a change log entry since a change log entry explains the state of things at one partiuclar revision, not for an indefinite time into the future. Perhaps we need some tool that shows relevant change log entries for each line in the code. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Class-level comments in the source code
At least for JavaScriptCore, I think that this is not a solvable problem. I suspect there are other parts of the code where this is true. There are many central classes that have such subtle behavior, which changes so often, that: - If you had a comment then it would be perpetually inaccurate. - If you tried to show change log entries for that class, you would be overwhelmed. I think if you want to understand many of the hairy parts of the code, you just need to read it. -F On Jul 6, 2012, at 4:11 PM, Ryosuke Niwa wrote: On Jul 6, 2012 3:04 PM, Filip Pizlo fpi...@apple.com wrote: It is indeed a problem of incentives. Nobody has the incentive to maintain a class comment when making changes, since comments are not checked by the compiler. Therefore, it's much better to not write comments, and instead find other ways of making the code legible. I think explaning the relationship between classes for ones that are not self-explanatory will be helpful and will probably incur very little cost compared to comments in functions. But I do agree that keeping them up-to-date is very challenging. This is why I usually prefer keeping such comments in a change log entry since a change log entry explains the state of things at one partiuclar revision, not for an indefinite time into the future. Perhaps we need some tool that shows relevant change log entries for each line in the code. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Jul 6, 2012 4:09 PM, Benjamin Poulain benja...@webkit.org wrote: On Fri, Jul 6, 2012 at 3:49 PM, Ryosuke Niwa rn...@webkit.org wrote: It DOES. Even today, I find many comments (even FIXMEs) that are out-of-date. Say you add a some comment to a function A because of some odd behavior of another function B, which A calls. Someone awesome later gets rid of the odd behavior in the function B by modifying yet another function C, which B calls. Now, how is this awesome person supposed to know to update the comment in A? Although I mostly agree with Ryosuke on comments... Since this subject comes from time to time, shouldn't we try having class comments as an experiment the help newcomers? If someone can come with good description for a class, review it. If class comments become harmful, they are easy to remove. I doubt many classes will benefit from such comments anyway. Yes, the particular problem I outlined here tends not to apply to class-level comments. At the end of the day, the amount of comments allowed in each patch should be decided on case-by-case basis. I have asked contributors to add comments for some complicated bidirectional text related functions because the problem we were solving itself was inherently complicated. I just wanted to point out that renaming refactoring should come first before adding comments. Just to illustrate how importent this is, I once saw comments like this: m_something; // somethingElse in HTML5 The fact the variable name didn't match the HTML5 terminology made it very hard to read the code (especially because it had a different tense and used a word that was used to describe a different thing in the spec) that used m_something. The code got significantly easier to read understand once I renamed m_something to m_somethingElse without any comments. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Class-level comments in the source code
On Jul 6, 2012, at 2:37 PM, Per Bothner per.both...@oracle.com wrote: On 07/06/2012 02:02 PM, Maciej Stachowiak wrote: [... reasonable stuff I fully agree with - but one question ...] Documenting ownership and lifetime relationships is also useful. We have tended to do that as diagrams separate from the code, since the places where it is most needed tend to be complicated clusters of related objects. Wouldn't such diagrams be harder to keep up-to-date than comments? Not necessarily. When you change an ownership relationship, you may have to update information about several classes, some of which may not have their code directly changed in the patch that modifies ownership model. In such cases, it would be easier to change one document than to find and update all the affected places. And if you fail to find one of the relevant places, you may end up with a false comment about ownership, which is more dangerous than no comment at all. That being said, I have not seriously tried either approach in WebKit so it's hard to predict which would be better in practice. Regards, Maciej ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Class-level comments in the source code
On Jul 6, 2012 4:29 PM, Maciej Stachowiak m...@apple.com wrote: On Jul 6, 2012, at 2:18 PM, Ryosuke Niwa rn...@webkit.org wrote: On Fri, Jul 6, 2012 at 1:51 PM, Per Bothner per.both...@oracle.com wrote: Documenting what a function/method does is sometimes useful, but what is really important is documenting what (an instance of) a class *is* and (preferably) how it relates to other objects. For some classes, yes. On Fri, Jul 6, 2012 at 2:02 PM, Maciej Stachowiak m...@apple.com wrote: On Jul 6, 2012, at 12:54 PM, Per Bothner per.both...@oracle.com wrote: The biggest annoyance I found is lack of class-level comments. For example what is an Interpreter? How many instances are there in the system? (I.e. is it a singleton class? Is there one per window? One per thread?) What is the relationship to JSGlobalData, JSGlobalObject, RootObject. There are a lot of these classes, and it takes quite a bit of staring at the code to figure it out. Worse, it's hard to remember it all, so if I come back to the codebase after working on something else I have to figure out all out again: I might remember some aspects (like a class starting with JS is probably some kind of JavaScript object), but not a lot of other relationships and properties of the classes. My recollection is that in past discussions, most folks were in favor of class-level comments that describe the purpose and nature of the class, particularly if not self-evident from the name. I think we'd likely take patches to take such comments. We also generally like why comments inside functions - comments that explain why something is done that way. ... Class-level comments don't really fall into these categories, and certainly the purpose of some of the objects you mention is not self-evident from the name (as with e.g. AffineTransform). I'd argue that we should still strive to make class names self-explanatory as much as possible, and use comments as the last resort. I've found that our culture of not adding comments have given me a pressure to think really hard to come up with better class names rather than going with vague names with explanatory comments. In some cases, it made me realize that the particular object relationships I had in my mind was not a good design and made me come up with a new design that involved easier-to-understand classes. I agree that self-explanatory class names are best. On the other hand, sometimes it's hard to make a name that is super clear but reasonably concise. For example, I'm not sure how I'd improve the name of JSGlobalData to make it sufficiently clear. What's really need is a why explanation, not a what explanation - why you need this special global data object. That seems like a good use for a comment. On the other hand, commenting AffineTransform with // This object represents an affine transform in 2D space is probably not super helpful. Yes, I agree. As I said on the other thread, reviewers should make a good judgement call as to whether we should be adding a comment or not. The comment about there's one to one mapping between threads and JSGlobalData is helpful for example. One big question I have is to where we should document inrer-class relationship. g.g. I've always thought we need a some comment somewhere explaining the differences between Range, Position, VisiblePosition, RenderedPosition, VisibleSelection, and FrameSelection but I don't know to where I should put it. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Class-level comments in the source code
On Jul 6, 2012 4:34 PM, Maciej Stachowiak m...@apple.com wrote: On Jul 6, 2012, at 2:37 PM, Per Bothner per.both...@oracle.com wrote: On 07/06/2012 02:02 PM, Maciej Stachowiak wrote: [... reasonable stuff I fully agree with - but one question ...] Documenting ownership and lifetime relationships is also useful. We have tended to do that as diagrams separate from the code, since the places where it is most needed tend to be complicated clusters of related objects. Wouldn't such diagrams be harder to keep up-to-date than comments? Not necessarily. When you change an ownership relationship, you may have to update information about several classes, some of which may not have their code directly changed in the patch that modifies ownership model. In such cases, it would be easier to change one document than to find and update all the affected places. And if you fail to find one of the relevant places, you may end up with a false comment about ownership, which is more dangerous than no comment at all. That being said, I have not seriously tried either approach in WebKit so it's hard to predict which would be better in practice. I've seen deceiving comments like that in the past. Even for external documentation, how do you suppose we keep them up to date? I'm frequently annoyed by WebKit wiki pages providing false information already (of course, I try to fix them as I encounter). - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On 07/06/2012 03:49 PM, Ryosuke Niwa wrote: If nothing else it's a useful sanity check: If something changes enough that you need to change a comment, that suggests it's a good thing to take the time to think through the implications well enough to write them down. We do that in change logs. Again: Differing cultures - it seems like WebKit treats change log comments as more important than I'm used to in other projects. I would never expect to read ChangeLog to get at the big picture; you'd get drowned in a mass of details, and changes that get reverted. I'd read ChangeLog s to answer a particular question like: is it deliberate that the code does this, and why. Though again, I'm used to the philosophy that if the code is obvious, don't comment, but if it is non-obvious explain why - in the code. That's where it is more likely to be correct, and more likely to updated when something changes. But I guess most of WebKit is relatively simple compared to (for example) the compilers that I've mostly worked on. -- --Per Bothner per.both...@oracle.com p...@bothner.com http://per.bothner.com/ ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Fri, Jul 6, 2012 at 4:54 PM, Per Bothner per.both...@oracle.com wrote: On 07/06/2012 03:49 PM, Ryosuke Niwa wrote: If nothing else it's a useful sanity check: If something changes enough that you need to change a comment, that suggests it's a good thing to take the time to think through the implications well enough to write them down. We do that in change logs. Again: Differing cultures Indeed. You just need to get used to it. *And please, please go read webkit-dev archives before continuing the discussion*. I suspect there are many contributors who are ignoring this thread because we have this discussion every six months or so, and they're getting tired of it. I'm getting tired of it as well. - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On Fri, Jul 6, 2012 at 7:59 PM, Ryosuke Niwa rn...@webkit.org wrote: On Fri, Jul 6, 2012 at 4:54 PM, Per Bothner per.both...@oracle.comwrote: On 07/06/2012 03:49 PM, Ryosuke Niwa wrote: If nothing else it's a useful sanity check: If something changes enough that you need to change a comment, that suggests it's a good thing to take the time to think through the implications well enough to write them down. We do that in change logs. Again: Differing cultures Indeed. You just need to get used to it. *And please, please go read webkit-dev archives before continuing the discussion*. I suspect there are many contributors who are ignoring this thread because we have this discussion every six months or so, and they're getting tired of it. I'm getting tired of it as well. e.g. http://lists.webkit.org/pipermail/webkit-dev/2012-March/020065.html http://lists.webkit.org/pipermail/webkit-dev/2011-July/017417.html http://lists.webkit.org/pipermail/webkit-dev/2011-March/016342.html http://lists.webkit.org/pipermail/webkit-dev/2011-January/015767.html http://lists.webkit.org/pipermail/webkit-dev/2010-February/011673.html http://lists.webkit.org/pipermail/webkit-dev/2009-August/009625.html - Ryosuke ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)
On 07/06/2012 08:23 PM, Ryosuke Niwa wrote: Again: Differing cultures Indeed. You just need to get used to it. The reasonable man adapts himself to the world; the unreasonable one persists in trying to adapt the world to himself. Therefore, all progress depends on the unreasonable man. --G. B. Shaw However, I don't feel strongly enough about WebKit to be unreasonable about it :-) *And please, please go read webkit-dev archives before continuing the discussion*. I suspect there are many contributors who are ignoring this thread because we have this discussion every six months or so, and they're getting tired of it. I'm getting tired of it as well. I've been subscribed to webkit-dev since March 2011, and I don't remember such a discussion. I could have missed it; webkit is somewhat on the periphery of my interests. e.g. http://lists.webkit.org/pipermail/webkit-dev/2012-March/020065.html http://lists.webkit.org/pipermail/webkit-dev/2011-July/017417.html http://lists.webkit.org/pipermail/webkit-dev/2011-March/016342.html http://lists.webkit.org/pipermail/webkit-dev/2011-January/015767.html http://lists.webkit.org/pipermail/webkit-dev/2010-February/011673.html http://lists.webkit.org/pipermail/webkit-dev/2009-August/009625.html These all seem to be about ChangeLogs or commit messages. Thus irrelevant to my point (which a number of people have agreed with) that class-level comments are often be useful and should be encouraged rather than discouraged (on a case-by-case basis, assuming they say something non-obvious). Regardless, I've said my piece. and have no interest in continuing. -- --Per Bothner per.both...@oracle.com p...@bothner.com http://per.bothner.com/ ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
