Bug 129058 – Move to using std::unique_ptr for Element, Node and related classes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Zan Dobersek]

Move to using std::unique_ptr for Element, Node and related classes

[Zan Dobersek]

Created attachment 224671 [details]
Patch

[WebKit Commit Bot]

Attachment 224671 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/ElementIteratorAssertions.h:57:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Zan Dobersek]

Created attachment 224672 [details]
Patch

[WebKit Commit Bot]

Attachment 224672 [details] did not pass style-queue:


ERROR: Source/WebCore/dom/ElementIteratorAssertions.h:57:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 224672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224672&action=review

> Source/WebCore/dom/Node.cpp:344
> -        data = ElementRareData::create(toRenderElement(m_data.m_renderer)).leakPtr();
> +        data = new ElementRareData(toRenderElement(m_data.m_renderer));
>      else
> -        data = NodeRareData::create(m_data.m_renderer).leakPtr();
> +        data = new NodeRareData(m_data.m_renderer);

I think you should use std::make_unique with release() here to avoid the ugly new expressions.

[Zan Dobersek]

Comment on attachment 224672 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=224672&action=review

>> Source/WebCore/dom/Node.cpp:344
>> +        data = new NodeRareData(m_data.m_renderer);
> 
> I think you should use std::make_unique with release() here to avoid the ugly new expressions.

Should this be a general rule or just a one-time exception? Should we rigorously try to remove any use of new?

[Zan Dobersek]

Comment on attachment 224672 [details]
Patch

Clearing flags on attachment: 224672

Committed r164638: <http://trac.webkit.org/changeset/164638>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Brent Fulgham]

This broke the Windows build:

    1>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\dom\DocumentOrderedMap.cpp(227): error C2280: 'std::unique_ptr<WebCore::NoEventDispatchAssertion,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function
                 with
                 [
                     _Ty=WebCore::NoEventDispatchAssertion
                 ] (..\dom\DOMAllInOne.cpp)
                 C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\memory(1486) : see declaration of 'std::unique_ptr<WebCore::NoEventDispatchAssertion,std::default_delete<_Ty>>::unique_ptr'
                 with
                 [
                     _Ty=WebCore::NoEventDispatchAssertion
                 ]
                 This diagnostic occurred in the compiler generated function 'WebCore::ElementIteratorAssertions::ElementIteratorAssertions(const WebCore::ElementIteratorAssertions &)'

[Brent Fulgham]

(In reply to comment #9)
> This broke the Windows build:
> 
>     1>c:\cygwin\home\buildbot\slave\win-debug\build\source\webcore\dom\DocumentOrderedMap.cpp(227): error C2280: 'std::unique_ptr<WebCore::NoEventDispatchAssertion,std::default_delete<_Ty>>::unique_ptr(const std::unique_ptr<_Ty,std::default_delete<_Ty>> &)' : attempting to reference a deleted function
>                  with
>                  [
>                      _Ty=WebCore::NoEventDispatchAssertion
>                  ] (..\dom\DOMAllInOne.cpp)
>                  C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\include\memory(1486) : see declaration of 'std::unique_ptr<WebCore::NoEventDispatchAssertion,std::default_delete<_Ty>>::unique_ptr'
>                  with
>                  [
>                      _Ty=WebCore::NoEventDispatchAssertion
>                  ]
>                  This diagnostic occurred in the compiler generated function 'WebCore::ElementIteratorAssertions::ElementIteratorAssertions(const WebCore::ElementIteratorAssertions &)'

It's trying to copy the unique_ptr in this routine.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 129321

[Brent Fulgham]

Windows is complaining because it doesn't like this line:

        auto it = entry.element ? elementDescandents.beginAt(*entry.element) : elementDescandents.begin();

You can establish the value of 'it' using either elementDescandents.begin(), or elementDescandents.beginAt(*entry.element), and everything is fine. But if you attempt to use the ternary operator you get the build error.

If you attempt to do it without the ternary operator:

auto it = elementDescandents.begin();
if (entry.element)
    it = elementDescandents.beginAT(*entryElement)

You also get the error.

[Darin Adler]

This was all done eventually; the code in question is gone.