Bug 139621

Summary: Move JavaScriptCore/yarr to std::unique_ptr
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: JavaScriptCoreAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, buildbot, commit-queue, darin, fpizlo, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 128007, 139983    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews100 for mac-mountainlion
none
Archive of layout-test-results from ews106 for mac-mountainlion-wk2
none
Patch
none
Patch
none
Patch none

Description Gyuyoung Kim 2014-12-13 08:00:34 PST
SSIA
Comment 1 Gyuyoung Kim 2014-12-13 08:02:21 PST
Created attachment 243256 [details]
Patch
Comment 2 Build Bot 2014-12-13 08:43:57 PST
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.
Comment 3 Build Bot 2014-12-13 08:44:00 PST
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
Comment 4 Build Bot 2014-12-13 08:46:09 PST
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.
Comment 5 Build Bot 2014-12-13 08:46:11 PST
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
Comment 6 Gyuyoung Kim 2014-12-25 23:57:26 PST
Created attachment 243759 [details]
Patch
Comment 7 Anders Carlsson 2014-12-28 09:03:32 PST
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));
Comment 8 Darin Adler 2014-12-28 11:10:13 PST
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.
Comment 9 Darin Adler 2014-12-28 11:10:41 PST
Adding Anders to the cc list so he sees my comments.
Comment 10 Gyuyoung Kim 2014-12-28 17:58:19 PST
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 ?
Comment 11 Gyuyoung Kim 2014-12-28 21:42:27 PST
Created attachment 243789 [details]
Patch
Comment 12 Gyuyoung Kim 2014-12-28 22:20:44 PST
Created attachment 243790 [details]
Patch
Comment 13 Gyuyoung Kim 2014-12-28 22:41:59 PST
(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.
Comment 14 WebKit Commit Bot 2014-12-29 18:59:33 PST
Comment on attachment 243790 [details]
Patch

Clearing flags on attachment: 243790

Committed r177820: <http://trac.webkit.org/changeset/177820>
Comment 15 WebKit Commit Bot 2014-12-29 18:59:37 PST
All reviewed patches have been landed.  Closing bug.