Switch DOMNodeList::item() and DOMNamedNodeMap::item() to fast ZPP #11361
Conversation
Code size impact: * DOMNodeList::item() => +96 bytes hot, +31 bytes cold * DOMNamedNodeMap::item() => +57 bytes hot, +31 bytes cold
| } | ||
| ZEND_PARSE_PARAMETERS_START(1, 1) | ||
| Z_PARAM_LONG(index) | ||
| ZEND_PARSE_PARAMETERS_END(); |
There was a problem hiding this comment.
Unrelated, is it me or does this just do nothing when passing a negative index? :| Does the spec allow for asking a negative index or should this be changed to emit a warning?
There was a problem hiding this comment.
Yeah I also noticed this, and I'm afraid we can't change the behaviour without making a BC break :(
Spec link for item: https://www.w3.org/TR/2003/WD-DOM-Level-3-Core-20030226/DOM3-Core.html#core-ID-349467F9
The problem is that spec defines the index as an unsigned type...
If index is greater than or equal to the number of nodes in this map, this returns null.
and further down: The node at the indexth position in the map, or null if that is not a valid index.
So it sounds to me it shouldn't throw, but rather should return NULL.
There was a problem hiding this comment.
Right, normally what we do is that we used to emit an E_WARNING for negative indices, which got changed to ValueErrors in PHP 8.0.
I think changing this to emit a warning for negative indices is at least a good start to indicate there is some mistake. But that's another PR to be done, (and probably check that we are handling indices consistently too...).
There was a problem hiding this comment.
OTOH, people coming from only knowing the DOM spec will be surprised that this throws. Spec doesn't define exceptions for this.
I tried with javascript, try executing this: document.getElementsByTagName('p').item(-1) it just returns NULL without an exception.
There was a problem hiding this comment.
Huh, welp IDK then. But in the other method we do check for the offset so :|
Code size impact: