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

[Per Bothner]

On 07/06/2012 02:02 PM, Ryosuke Niwa wrote:
> As Eric pointed out, this is completely off the topic of the thread now.
> So moving to a new thread.

[Alas we now have 3 new treads ...]

>     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.

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.  Good class
and method names are very valuable, but so are well-chosen comments.

>     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.

> Now, it seems like you do work on JSC, and I
> understand JS JIT and JS interpreters could be very complicated.

Yep.  I certainly don't claim to understand all or even most of it.

> If you think the code in JSC or other parts of WebKit, then please file
> bugs to refactor such code to make them more self-explanatory. I hope we
> can both agree that code such as:
> Node* highestAncestor(Node* node)
> {
>      ASSERT(node);
>      Node* parent = node;
>      while (node = node->parentNode())
>          parent = node;
>      return parent;
> }
> doesn't need any comments. We should strive to make all code as
> self-evident as this one.

I agree - at least I can't think of anything useful to add.

>     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?)
>
>
> Okay, I had never heard of this class so I opened up
> JavaScriptCore.xcodeproj and searched for "new
> Interpreter". JSGlobalData has it as a member so clearly it's not a
> singleton class. And there's a nice comment at the beginning of the
> class declaration of JSGlobalData that says WebCore has a one-to-one
> mapping of threads to JSGlobalData.

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.  So you're tempted to submit a patch so you
didn't have to do this next time you want to figure it out - assuming
you feel such patches are welcome (and you've worked out an acceptable
procedure with your employer for submitting patches ...).

> I agree that WebKit codebase may not be the most friendly project to
> someone who is new to the project.
>
> On the other hand, we don't want to have valuable contributors spend all
> their time updating & adding comments. And the lack of comments doesn't
> seem to be a show stopper for people who frequently contribute and make
> significant contributions to the project.

We all have limited resources, and must prioritize.  But I think keeping
useful comments up-to-date shouldn't take a lot of time.  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.
-- 
	--Per Bothner
per.bothner at oracle.com   per at bothner.com   http://per.bothner.com/