Bug 139621

Created attachment 243256 [details] Patch

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.

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

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

Created attachment 243759 [details] Patch

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

Adding Anders to the cc list so he sees my comments.

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 ?

Created attachment 243789 [details] Patch

Created attachment 243790 [details] Patch

(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 on attachment 243790 [details] Patch Clearing flags on attachment: 243790 Committed r177820: <http://trac.webkit.org/changeset/177820>

All reviewed patches have been landed. Closing bug.