[webkit-reviews] review denied: [Bug 3539] Array join and toString methods do not support circular references : [Attachment 4532] Reimplemented patch with HashSet

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Oliver Hunt
<ojh16 at student.canterbury.ac.nz>'s request for review:
Bug 3539: Array join and toString methods do not support circular references
http://bugzilla.opendarwin.org/show_bug.cgi?id=3539

Attachment 4532: Reimplemented patch with HashSet
http://bugzilla.opendarwin.org/attachment.cgi?id=4532&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
I don't understand why it's necessary to copy thisObj into a second local
variable currentRef. Lets just use thisObj directly unless there's a reason not
to.

I think it would be easier to read this if the code said:

    if (visitedElems.contains(thisObj))
	return String("");

before anything else (even before declaring separator and str), rather than
indenting all the code inside an if statement. That would also probably make
the diff smaller and easier to verify.

Also, I think the HashSet should be:

    HashSet< ObjectImp*, PointerHash<ObjectImp*> >

There's an advantage to HashSet<void*>, in that we're guaranteed all such sets
are a single template expansion, preventing any possible code bloat. But a
disadvantage to HashSet<void*> is that without type safety, it's possible to
make certain mistakes with a set (for example, adding and removing the same
object, but a pointer to a different part in the case of multiple inheritance).
In this case, I think the code is simple enough that I don't think there are
any real problems, but I'd prefer to get off on the right foot using this
class.

There are a few minor formatting problems, for example:

    ObjectImp *currentRef=thisObj;

the above needs spaces around the "=". And:

+    if(!visitedElems.contains(currentRef)) {	 

the above needs a space after the if and before the opening parenthesis. And:

+	 if ( exec->hadException() )
+	   break;

if you're going to modify the code, you should fix the format of that by
removing the extra spaces inside the parentheses.

Also, there are a number of 1-line if and else statements with braces; style
guide says not to use braces in these cases.