Bug 139621 – Move JavaScriptCore/yarr to std::unique_ptr

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

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 243256 [details]
Patch

[Build Bot]

Comment on attachment 243256 [details]
Patch

Attachment 243256 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5965336272699392

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 243257 [details]
Archive of layout-test-results from ews100 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 243256 [details]
Patch

Attachment 243256 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6544379468578816

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 243258 [details]
Archive of layout-test-results from ews106 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Gyuyoung Kim]

Created attachment 243759 [details]
Patch

[Anders Carlsson]

Comment on attachment 243759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243759&action=review

> Source/JavaScriptCore/yarr/YarrPattern.h:271
>          PatternAlternative* alternative = new PatternAlternative(this);
> -        m_alternatives.append(adoptPtr(alternative));
> +        m_alternatives.append(std::unique_ptr<PatternAlternative>(alternative));

You can write this as

m_alternatives.append(std::make_unique<PatternAlternative>(this));

[Darin Adler]

Comment on attachment 243759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243759&action=review

>> Source/JavaScriptCore/yarr/YarrPattern.h:271
>> +        m_alternatives.append(std::unique_ptr<PatternAlternative>(alternative));
> 
> You can write this as
> 
> m_alternatives.append(std::make_unique<PatternAlternative>(this));

If we do that, where do we get the return value? It seems wasteful to have to write:

    return m_alternatives.last().get();

But I think that’s what would be required. Unless we change Vector::append to return a reference to the thing that was appended. Then it could be:

    return m_alternatives.append(std::make_unique<PatternAlternative>(this)).get();

> Source/JavaScriptCore/yarr/YarrPattern.h:340
> -            m_userCharacterClasses.append(adoptPtr(newlineCached = newlineCreate()));
> +            m_userCharacterClasses.append(std::unique_ptr<CharacterClass>(newlineCached = newlineCreate()));

The right way to do this is to change the create function to return a unique_ptr, not to adopt it from the returned pointer. But we have the same issue here with getting the pointer value as in the other Vector::append call above.

[Darin Adler]

Adding Anders to the cc list so he sees my comments.

[Gyuyoung Kim]

Comment on attachment 243759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=243759&action=review

>>> Source/JavaScriptCore/yarr/YarrPattern.h:271
>>> +        m_alternatives.append(std::unique_ptr<PatternAlternative>(alternative));
>> 
>> You can write this as
>> 
>> m_alternatives.append(std::make_unique<PatternAlternative>(this));
> 
> If we do that, where do we get the return value? It seems wasteful to have to write:
> 
>     return m_alternatives.last().get();
> 
> But I think that’s what would be required. Unless we change Vector::append to return a reference to the thing that was appended. Then it could be:
> 
>     return m_alternatives.append(std::make_unique<PatternAlternative>(this)).get();

If we used std::make_unique<> instead of std::unique_ptr, layout test was broken. You can see the broken in previous patch. That's why I use std::unique_ptr<> instead of std::make_unique<>. BTW, should I change that Vector class  returns a reference in this patch ?

[Gyuyoung Kim]

Created attachment 243789 [details]
Patch

[Gyuyoung Kim]

Created attachment 243790 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #8)
> Comment on attachment 243759 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243759&action=review
> 
> >> Source/JavaScriptCore/yarr/YarrPattern.h:271
> >> +        m_alternatives.append(std::unique_ptr<PatternAlternative>(alternative));
> > 
> > You can write this as
> > 
> > m_alternatives.append(std::make_unique<PatternAlternative>(this));
> 
> If we do that, where do we get the return value? It seems wasteful to have
> to write:
> 
>     return m_alternatives.last().get();
> 
> But I think that’s what would be required. Unless we change Vector::append
> to return a reference to the thing that was appended. Then it could be:
> 
>     return
> m_alternatives.append(std::make_unique<PatternAlternative>(this)).get();

In latest patch, I use below codes as you suggested.

    {
        m_alternatives.append(std::make_unique<PatternAlternative>(this));
        return static_cast<PatternAlternative*>(m_alternatives.last().get());
    }



> > Source/JavaScriptCore/yarr/YarrPattern.h:340
> > -            m_userCharacterClasses.append(adoptPtr(newlineCached = newlineCreate()));
> > +            m_userCharacterClasses.append(std::unique_ptr<CharacterClass>(newlineCached = newlineCreate()));
> 
> The right way to do this is to change the create function to return a
> unique_ptr, not to adopt it from the returned pointer. But we have the same
> issue here with getting the pointer value as in the other Vector::append
> call above.

To change to return a unique_ptr in fooCreate(), I have to modify "create_regex_tables". I would do this after landing this patch in Bug 139983.

[WebKit Commit Bot]

Comment on attachment 243790 [details]
Patch

Clearing flags on attachment: 243790

Committed r177820: <http://trac.webkit.org/changeset/177820>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.