Bug 139983

Summary: Change to return a unique_ptr<> in fooCreate()
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: JavaScriptCoreAssignee: Gyuyoung Kim <gyuyoung.kim>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, sam
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 139621    
Bug Blocks:    
Attachments:
Description Flags
WIP
none
Patch
none
Patch none

Description Gyuyoung Kim 2014-12-28 22:40:54 PST
As suggested in https://bugs.webkit.org/show_bug.cgi?id=139621#c8, fooCreate() needs to return std::unique_ptr<> in YarrPattern.h
Comment 1 Gyuyoung Kim 2014-12-30 08:00:21 PST
Created attachment 243824 [details]
WIP
Comment 2 Gyuyoung Kim 2014-12-30 23:35:33 PST
Created attachment 243834 [details]
Patch
Comment 3 Sam Weinig 2014-12-31 13:06:25 PST
This seems to also remove the caching. Is that intentional? If so, why?
Comment 4 Darin Adler 2014-12-31 20:44:55 PST
Comment on attachment 243834 [details]
Patch

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

> Source/JavaScriptCore/yarr/YarrPattern.h:-320
> -        newlineCached = 0;
> -        digitsCached = 0;
> -        spacesCached = 0;
> -        wordcharCached = 0;
> -        nondigitsCached = 0;
> -        nonspacesCached = 0;
> -        nonwordcharCached = 0;

These should not be deleted.

> Source/JavaScriptCore/yarr/YarrPattern.h:331
> -        if (!newlineCached)
> -            m_userCharacterClasses.append(std::unique_ptr<CharacterClass>(newlineCached = newlineCreate()));
> -        return newlineCached;
> +        m_userCharacterClasses.append(newlineCreate());
> +        return m_userCharacterClasses.last().get();

This is wrong. It gets rid of the caching and creates a new character class every time this function is called. We still want the caching. The code should be:

    if (!newlineCached) {
        m_userCharacterClasses.append(newlineCreate());
        newlineCached = m_userCharacterClasses.last().get();
    }
    return newlineCached;

Same for the other functions below.

> Source/JavaScriptCore/yarr/YarrPattern.h:-399
> -    CharacterClass* newlineCached;
> -    CharacterClass* digitsCached;
> -    CharacterClass* spacesCached;
> -    CharacterClass* wordcharCached;
> -    CharacterClass* nondigitsCached;
> -    CharacterClass* nonspacesCached;
> -    CharacterClass* nonwordcharCached;

These must not be deleted.
Comment 5 Gyuyoung Kim 2015-01-01 00:47:41 PST
Created attachment 243859 [details]
Patch
Comment 6 Gyuyoung Kim 2015-01-01 00:48:47 PST
(In reply to comment #4)
> Comment on attachment 243834 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243834&action=review
> 
> > Source/JavaScriptCore/yarr/YarrPattern.h:-320
> > -        newlineCached = 0;
> > -        digitsCached = 0;
> > -        spacesCached = 0;
> > -        wordcharCached = 0;
> > -        nondigitsCached = 0;
> > -        nonspacesCached = 0;
> > -        nonwordcharCached = 0;
> 
> These should not be deleted.
> 
> > Source/JavaScriptCore/yarr/YarrPattern.h:331
> > -        if (!newlineCached)
> > -            m_userCharacterClasses.append(std::unique_ptr<CharacterClass>(newlineCached = newlineCreate()));
> > -        return newlineCached;
> > +        m_userCharacterClasses.append(newlineCreate());
> > +        return m_userCharacterClasses.last().get();
> 
> This is wrong. It gets rid of the caching and creates a new character class
> every time this function is called. We still want the caching. The code
> should be:
> 
>     if (!newlineCached) {
>         m_userCharacterClasses.append(newlineCreate());
>         newlineCached = m_userCharacterClasses.last().get();
>     }
>     return newlineCached;
> 
> Same for the other functions below.
> 
> > Source/JavaScriptCore/yarr/YarrPattern.h:-399
> > -    CharacterClass* newlineCached;
> > -    CharacterClass* digitsCached;
> > -    CharacterClass* spacesCached;
> > -    CharacterClass* wordcharCached;
> > -    CharacterClass* nondigitsCached;
> > -    CharacterClass* nonspacesCached;
> > -    CharacterClass* nonwordcharCached;
> 
> These must not be deleted.

I'm sorry to miss the caching. Fixed all. Thanks a lot !
Comment 7 Darin Adler 2015-01-01 11:04:08 PST
Comment on attachment 243859 [details]
Patch

Looks good. I’d still like to optimize the last().get() idiom here by adding a return value to Vector::append; it would also make all those if statement bodies be single lines. Did you file a bug about doing that?
Comment 8 Gyuyoung Kim 2015-01-01 16:31:59 PST
(In reply to comment #7)
> Comment on attachment 243859 [details]
> Patch
> 
> Looks good. I’d still like to optimize the last().get() idiom here by adding
> a return value to Vector::append; it would also make all those if statement
> bodies be single lines. Did you file a bug about doing that?

Not yet. I'm going to file a bug for it. Let me CC you the bug.
Comment 9 Gyuyoung Kim 2015-01-01 16:32:48 PST
Comment on attachment 243859 [details]
Patch

efl-ews was false alarm.
Comment 10 WebKit Commit Bot 2015-01-01 17:13:23 PST
Comment on attachment 243859 [details]
Patch

Clearing flags on attachment: 243859

Committed r177854: <http://trac.webkit.org/changeset/177854>
Comment 11 WebKit Commit Bot 2015-01-01 17:13:28 PST
All reviewed patches have been landed.  Closing bug.