[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] 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
