Bug 139983 – Change to return a unique_ptr<> in fooCreate()

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

[Gyuyoung Kim]

As suggested in https://bugs.webkit.org/show_bug.cgi?id=139621#c8, fooCreate() needs to return std::unique_ptr<> in YarrPattern.h

[Gyuyoung Kim]

Created attachment 243824 [details]
WIP

[Gyuyoung Kim]

Created attachment 243834 [details]
Patch

[Sam Weinig]

This seems to also remove the caching. Is that intentional? If so, why?

[Darin Adler]

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.

[Gyuyoung Kim]

Created attachment 243859 [details]
Patch

[Gyuyoung Kim]

(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 !

[Darin Adler]

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?

[Gyuyoung Kim]

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

[Gyuyoung Kim]

Comment on attachment 243859 [details]
Patch

efl-ews was false alarm.

[WebKit Commit Bot]

Comment on attachment 243859 [details]
Patch

Clearing flags on attachment: 243859

Committed r177854: <http://trac.webkit.org/changeset/177854>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.