Bug 109218 – Yarr: Use OwnPtr to make pattern/disjunction/character-class ownership clearer.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Andreas Kling]

Yarr could use a bit more OwnPtrs to simplify object lifetime management.
And while we're there, there are some easy memory wins in excessive Vector padding..

[Andreas Kling]

Created attachment 187149 [details]
Proposed patch

[Benjamin Poulain]

Comment on attachment 187149 [details]
Proposed patch

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.

[Andreas Kling]

(In reply to comment #2)
> (From update of attachment 187149 [details])
> 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?

Right. I couldn't think of anything better. One idea would be changing the argument name to reflect the swapping behavior; e.g "parenthesesInfoToAdopt" or such. Do note that this swap() is not really a crucial part of the patch, I added it mostly to be symmetrical with the other swap() that was happening in the same function. It's also worth noting that BytecodePattern is only constructed in this fashion (at the moment):

PassOwnPtr<BytecodePattern> byteCompile(YarrPattern& pattern, BumpPointerAllocator* allocator)
{
    return ByteCompiler(pattern).compile(allocator);
}

^and here, the vector getting moved/swapped is a member of the ByteCompiler class, so it is never really expected to survive compile(). Though I agree that it would be nice to be more future-proof. :/

> > 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.

True, I'll fix that.

> > 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.

My past self disagrees with your ugly and impure ideas. My present self agrees with you. Will fix!

[Andreas Kling]

Committed r143018: <http://trac.webkit.org/changeset/143018>

[Darin Adler]

*** Bug 73778 has been marked as a duplicate of this bug. ***