Summary: | Move JavaScriptCore/yarr to std::unique_ptr | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||
Component: | JavaScriptCore | Assignee: | Gyuyoung Kim <gyuyoung.kim> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | andersca, buildbot, commit-queue, darin, fpizlo, rniwa | ||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Bug Depends on: | |||||||||||||||||
Bug Blocks: | 128007, 139983 | ||||||||||||||||
Attachments: |
|
Description
Gyuyoung Kim
2014-12-13 08:00:34 PST
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. |