Bug 139983

Summary:

Change to return a unique_ptr<> in fooCreate()

Product:

WebKit

Reporter:

Gyuyoung Kim <gyuyoung.kim>

Component:

JavaScriptCore

Assignee:

Gyuyoung Kim <gyuyoung.kim>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, sam

Priority:

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.