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

[Maciej Stachowiak]

On Jul 12, 2012, at 6:50 AM, John Mellor <johnme at chromium.org> wrote:

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

I think documenting the public interfaces of core data structures is probably a worthwhile tradeoff, since they change rarely but are used in many places. In fact, ListHashSet already has a class comment and documents some method behavior that is not obvious from the signature. Covering the issue you mention seems worthwhile as well.

        https://bugs.webkit.org/show_bug.cgi?id=91106

Regards,
Maciej

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20120712/f5c9550e/attachment.html>