[webkit-reviews] review denied: [Bug 64202] Enh: Improve handling of RegExp in the form of /.*blah.*/ : [Attachment 100374] Updated patch to fix win compilation error.

[bugzilla-daemon at webkit.org]

Gavin Barraclough <barraclough at apple.com> has denied Michael Saboff
<msaboff at apple.com>'s request for review:
Bug 64202: Enh: Improve handling of RegExp in the form of /.*blah.*/
https://bugs.webkit.org/show_bug.cgi?id=64202

Attachment 100374: Updated patch to fix win compilation error.
https://bugs.webkit.org/attachment.cgi?id=100374&action=review

------- Additional Comments from Gavin Barraclough <barraclough at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=100374&action=review


> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:1074
> +	   if (pattern->m_multiline) {

These two if statements could be merged & save a repeating the match assign &
return true.

if (!pattern->m_multiline && ((term.anchors.m_bol && matchBegin) ||
(term.anchors.m_eol && matchEnd != input.end())))
    return false;

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:1379
> +	       BACKTRACK();

I don't think we should ever be able to backtrack into here; I think this
should be ASSERT_NOT_REACHED()?

> Source/JavaScriptCore/yarr/YarrJIT.cpp:1011
> +	   saveStartIndex.append(branch32(Equal, matchPos, Imm32(m_checked)));

Please modify per our discussion, /.*\n\d+.*/ "abc\n123".

> Source/JavaScriptCore/yarr/YarrPattern.cpp:716
> +	   for (size_t alt = 0; alt < alternatives.size(); ++alt) {

We shouldn't need a for-loop here, given the above guard!   possibly should
switch the check to "if (alternatives.size() != 1) return;" (I don't think we
ever have 0 alternatives, but just to be safe!), then drop the loop & just work
over alternatives[0].