As suggested in https://bugs.webkit.org/show_bug.cgi?id=139621#c8, fooCreate() needs to return std::unique_ptr<> in YarrPattern.h
Created attachment 243824 [details] WIP
Created attachment 243834 [details] Patch
This seems to also remove the caching. Is that intentional? If so, why?
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.
Created attachment 243859 [details] Patch
(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 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?
(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 on attachment 243859 [details] Patch efl-ews was false alarm.
Comment on attachment 243859 [details] Patch Clearing flags on attachment: 243859 Committed r177854: <http://trac.webkit.org/changeset/177854>
All reviewed patches have been landed. Closing bug.