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

[Ryosuke Niwa]

On Jul 6, 2012 3:16 PM, "Per Bothner" <per.bothner at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120706/0404e8ae/attachment.html>