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

[John Mellor]

On Wed, Jul 11, 2012 at 4:53 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:

>
> On Jul 11, 2012 8:43 AM, "John Mellor" <johnme at chromium.org> wrote:
> >
> > On Wed, Jul 11, 2012 at 4:21 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
> >>
> >> What's the point of adding this comment when the URL contains all the
> information?  All we need is the URL.  If anything, we should be describing
> the difference between the inline boxes in CSS2.1 and our implementation
> instead.
> >
> > That would be great! I agree that there's probably limited value in just
> copy/pasting from specs like I did. Linking to the spec something is based
> on and describing the differences would add a lot of value.
>
> The problem is that we'll then incur the maintenace cost of keeping
> comments up-to-date and the risk of them getting out-of-date as we have
> previously discussed.
>
> - Ryosuke
>
Like many others on this thread, I don't buy this argument. It's well known
that developers spend much more time reading & understanding code than
writing it. This is especially true in an inevitably complex codebase like
WebKit, with many interdependencies. Well chosen comments that add value
(like describing the deviations from a spec) are worth maintaining. Indeed,
12 months down the line, the original author will probably have forgotten
enough detail that they themselves would have benefited by appropriately
commenting their code.

Even to check simple things like whether some method can return null you
typically have to grep through all references to the thing being returned,
and often in turn through all references to those. It's not feasible for
developers to read the full transitive closure of code that interacts with
every class they use, so they skim it and make mistakes. These mistakes
often then slip through code review because they look reasonable. The bugs
caused by these incorrect assumptions (that wouldn't have occurred with
appropriate comments) seem much more concerning than the hypothetical bugs
caused by stale comments.

To take an arbitrary example, lets say that while iterating through a
ListHashSet something causes entries to be deleted. Intuitively it seems
this needn't invalidate the iterator, as long as the entry the iterator is
currently pointing to isn't removed. But is that actually the case in this
particular implementation? A well-documented library like Java's
LinkedHashSet<http://docs.oracle.com/javase/6/docs/api/java/util/LinkedHashSet.html>
will
warn you "if the set is modified at any time after the iterator is created,
in any way except through the iterator's own remove method, the iterator
will throw a ConcurrentModificationException" and that's that. I just tried
to find this out in WebKit and had to read though ListHashSetIterator,
ListHashSetConstIterator, ListHashSetNode, ListHashSet::remove,
ListHashSet::unlinkAndDelete, HashTable::remove,
HashTable::internalCheckTableConsistency,
HashTable::removeWithoutEntryConsistencyCheck,
HashTable::removeAndInvalidateWithoutEntryConsistencyCheck,
HashTable::invalidateIterators, HashTable::shrink, HashTable::rehash,
ListHashSet::add, HashTable::add,
HashTable::makeKnownGoodIterator, HashTableIterator, HashTable::AddResult
and ListHashSetTranslator::translate, and even learn about using the template
keyword<http://stackoverflow.com/questions/610245/where-and-why-do-i-have-to-put-the-template-and-typename-keywords>
for
disambiguation. Eventually there was enough information to conclude that
yes, it probably is safe since the ListHashSetNodes are allocated on the
heap by ListHashSetTranslator::translate, so even though the HashTable
invalidates its own iterators and HashTable::rehash may reallocate its
storage, the actual ListHashSetNode pointed to by ListHashSetConstInterator
should continue to exist. But constantly having to do such deep research
makes coding highly inefficient, and there's a high risk of making errors.

All the best,
John
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120712/4212452d/attachment.html>