<div class="gmail_extra">On Wed, Jul 11, 2012 at 4:53 PM, Ryosuke Niwa <span dir="ltr">&lt;<a href="mailto:rniwa@webkit.org" target="_blank">rniwa@webkit.org</a>&gt;</span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div class="im"><p><br>
On Jul 11, 2012 8:43 AM, &quot;John Mellor&quot; &lt;<a href="mailto:johnme@chromium.org" target="_blank">johnme@chromium.org</a>&gt; wrote:<br>
&gt;<br>
&gt; On Wed, Jul 11, 2012 at 4:21 PM, Ryosuke Niwa &lt;<a href="mailto:rniwa@webkit.org" target="_blank">rniwa@webkit.org</a>&gt; wrote:<br>
&gt;&gt;<br>
&gt;&gt; What&#39;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.<br>


&gt;<br>
&gt; That would be great! I agree that there&#39;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.</p>



</div><p>The problem is that we&#39;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.</p><span class=""><font color="#888888">
<p>- Ryosuke<br>
</p>
</font></span></blockquote></div><div>Like many others on this thread, I don&#39;t buy this argument. It&#39;s well known that developers spend much more time reading &amp; 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.</div>

<div><br></div><div>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&#39;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&#39;t have occurred with appropriate comments) seem much more concerning than the hypothetical bugs caused by stale comments.</div>

<div><br></div><div>To take an arbitrary example, lets say that while iterating through a ListHashSet something causes entries to be deleted. Intuitively it seems this needn&#39;t invalidate the iterator, as long as the entry the iterator is currently pointing to isn&#39;t removed. But is that actually the case in this particular implementation? A well-documented library like Java&#39;s <a href="http://docs.oracle.com/javase/6/docs/api/java/util/LinkedHashSet.html">LinkedHashSet</a> will warn you &quot;if the set is modified at any time after the iterator is created, in any way except through the iterator&#39;s own remove method, the iterator will throw a ConcurrentModificationException&quot; and that&#39;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 <a href="http://stackoverflow.com/questions/610245/where-and-why-do-i-have-to-put-the-template-and-typename-keywords">template keyword</a> 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&#39;s a high risk of making errors.</div>

<div><br></div><div>All the best,</div><div>John</div></div>