[webkit-dev] Comments in the code (Was Please include function-level comments in change log entries)

[Ryosuke Niwa]

On Jul 6, 2012 4:09 PM, "Benjamin Poulain" <benjamin at webkit.org> wrote:
>
> On Fri, Jul 6, 2012 at 3:49 PM, Ryosuke Niwa <rniwa at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120706/c53fede5/attachment.html>