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