[Webkit-unassigned] [Bug 22269] Reduce PropertyMap usage

[bugzilla-daemon at webkit.org]

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


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25174|review?                     |review+
               Flag|                            |




------- Comment #2 from darin at apple.com  2008-11-14 14:11 PDT -------
(From update of attachment 25174)
> +    RefPtr<StructureID> structureID = StructureID::addPropertyTransitionToExistingStructure(m_structureID, propertyName, attributes, offset);
> +    if (structureID) {

It would make the long line even longer, but this would be a great case for
putting the assignment into the if statement to make it clear you can't use
this outside the if statement. Also is this a rare enough case that it should
go into a separate function? Or maybe a separate inline function just for code
clarity?

> -    RefPtr<StructureID> structureID = StructureID::addPropertyTransition(m_structureID, propertyName, attributes, offset);
> +    structureID = StructureID::addPropertyTransition(m_structureID, propertyName, attributes, offset);

If you put the structureID into the if then you wouldn't have to change this
line, and in fact it would be ever so slightly more efficient because you'd
save one branch and deref.

> +    // Taken  from http://www.cs.utk.edu/~vose/c-stuff/bithacks.html

Extra space here in the comment.

> +    // Search for the last StructureID with a property table 

Please use periods if you use capitals.

> +            rehashPropertyMapHashTable(sizeForKeyCount(m_offset + 1)); // This could be made more efficient by combinding with the copy above 

Ditto. Also misspells combining.

> +    for (int i = structures.size() - 2; i >= 0; --i) {

ptrdiff_t instead of int? I can never figure out what we should do about 64 vs.
32 bit.

> +        structure->m_nameInPrevious->ref();
> +        PropertyMapEntry entry(structure->m_nameInPrevious, structure->m_offset, structure->m_attributesInPrevious, ++m_propertyTable->lastIndexUsed); 
> +        insertIntoPropertyMapHashTable(entry);

I wish there was a way to put that ref call somewhere lower level so it was
more obvious it was balanced by deref.

> +    // deleted offsets vector) before transistioning from dictionary. 

Spelling error in the word "transitioning".

Does this get thoroughly enough tested by existing tests, or do we need new
tests?

r=me


-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.