[webkit-reviews] review granted: [Bug 109218] Yarr: Use OwnPtr to make pattern/disjunction/character-class ownership clearer. : [Attachment 187149] Proposed patch

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has granted Andreas Kling
<akling at apple.com>'s request for review:
Bug 109218: Yarr: Use OwnPtr to make pattern/disjunction/character-class
ownership clearer.
https://bugs.webkit.org/show_bug.cgi?id=109218

Attachment 187149: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=187149&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=187149&action=review


This looks great.

The one thing I am not a big fan is the swap() of allParenthesesInfo. It now
clears the input vectors which could be unexpected for the caller of
BytecodePattern.
I guess we cannot use C++ move syntax here because of Windows. Do we have any
other way to specify the move semantic?

> Source/JavaScriptCore/yarr/YarrInterpreter.cpp:1714
> +	   parenthesesDisjunction->terms.reserveInitialCapacity(endTerm -
beginTerm + 1);

I think I'd add a new variable for "beginTerm + 1".
A +1 use twice out of nowhere could be an invitation for future disasters.

> Source/JavaScriptCore/yarr/YarrPattern.cpp:501
> +	   return m_pattern.m_disjunctions.last().get();

This is fairly ugly. Keeping the pointer around could be an option.

> Source/JavaScriptCore/yarr/YarrPattern.h:284
> +	   return m_alternatives.last().get();

Again, not a fan of this notation. I think it reads worse than keeping a
pointer separately.