I think the challenge in part is to explain why the ChangeLog is useful. Comments in there hopefully explain why as a guide to reviewers to give the reviewers and future onlookers a guide to the change. It feels like what comments are that useful but often get inserted in there to fill it out. Is there a good way to address this? (Should folks just leave what comments out or put in the obvious what?) Since we have an example ChangeLog, I'll point out what I mean using it (but it is evident in many ChangeLog's from me as well). What value does this comment add? - runtime/JSGlobalData.h: Replaced the HashSet and HashCountedSet with a Vector. On the other hand, I found this very informative: wtf/PassRefPtr.h: (WTF::PassRefPtr::~PassRefPtr): The most common thing to do with a PassRefPtr in hot code is to pass it and then destroy it once it's set to zero. Help the optimizer by telling it that's true. Here again, we see a what comment: parser/Nodes.cpp: (JSC::NodeReleaser::releaseAllNodes): ALWAYS_INLINE. (JSC::NodeReleaser::adopt): Ditto. Which doesn't tell me why ALWAYS_INLINE was added (which I can easily see from the patch). dave On Wed, Mar 21, 2012 at 2:33 PM, Maciej Stachowiak <mjs@apple.com> wrote:
On Mar 21, 2012, at 2:29 PM, Timothy Hatcher wrote:
Lately I have observed more <http://trac.webkit.org/changeset/111595> and more <http://trac.webkit.org/changeset/111577> and more<http://trac.webkit.org/changeset/111527> changes going into WebKit that lack any details about why a particular change was made. It is intended that the ChangeLog (and commit message) contain some details about your change, not just the bug title and URL.
The contributing information<http://www.webkit.org/coding/contributing.html#changelogs> on subject is pretty sparse, which has likely caused this problem to manifest. However, the example ChangeLog <http://trac.webkit.org/changeset/43259> linked from that page is prime example of what we all should strive for when describing our changes.
To help curb this lack of detail in ChangeLogs, I propose we add script<https://bugs.webkit.org/show_bug.cgi?id=81828> (or augment an existing script) to check for this missing information and inform the contributor. It is clear not all reviewers are asking patch authors to provide this information when reviewing, and such a tool would help enforce it.
I think this is a reasonable suggestion. I think we should make the contributor docs more clear about what is expected, as well.
Regards, Maciej
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev