1250196 - Latest Skia import breaks builds with libc++

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[(no longer active)]

We do scary things such as |#define forward Forward| and |#define unique_ptr UniquePtr|.  The problem with the former is that libc++ puts forward in std::__v1, and not std, and the problems with the latter is that we don't import mozilla::Forward into std, and also our UniquePtr implementation isn't compatible with std::unique_ptr because it lacks get_deleter().  These symptoms cause error messages such as the following when building against libc++:

In file included from /Users/ehsan/moz/src/gfx/2d/DrawTargetSkia.cpp:6:
In file included from /Users/ehsan/moz/src/gfx/2d/DrawTargetSkia.h:10:
In file included from /Users/ehsan/moz/src/gfx/2d/HelpersSkia.h:17:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/vector:274:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__split_buffer:634:31: error: no template named 'Forward' in namespace
      'std::__1'; did you mean simply 'Forward'?
                              _VSTD::forward<_Args>(__args)...);
                              ^~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__config:366:15: note: expanded from macro '_VSTD'
#define _VSTD std::_LIBCPP_NAMESPACE
              ^
/Users/ehsan/moz/src/obj-ff-clang-plugin.noindex/dist/include/mozilla/Move.h:212:1: note: 'Forward' declared here
Forward(typename RemoveReference<T>::Type& aX)
^
In file included from /Users/ehsan/moz/src/gfx/2d/DrawTargetSkia.cpp:6:
In file included from /Users/ehsan/moz/src/gfx/2d/DrawTargetSkia.h:10:
In file included from /Users/ehsan/moz/src/gfx/2d/HelpersSkia.h:17:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/vector:274:
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__split_buffer:634:31: error: no template named 'Forward' in namespace
      'std::__1'; did you mean simply 'Forward'?
                              _VSTD::forward<_Args>(__args)...);
                              ^~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/__config:366:15: note: expanded from macro '_VSTD'
#define _VSTD std::_LIBCPP_NAMESPACE
              ^
/Users/ehsan/moz/src/obj-ff-clang-plugin.noindex/dist/include/mozilla/Move.h:212:1: note: 'Forward' declared here
Forward(typename RemoveReference<T>::Type& aX)
^
(followed by many more), and:

In file included from /Users/ehsan/moz/src/gfx/2d/Factory.cpp:47:
In file included from /Users/ehsan/moz/src/gfx/2d/DrawTargetRecording.h:11:
In file included from /Users/ehsan/moz/src/gfx/2d/DrawEventRecorder.h:12:
/Users/ehsan/Downloads/clang+llvm-3.7.0-x86_64-apple-darwin/bin/../include/c++/v1/fstream:566:9: error: no template named 'UniquePtr'; did you mean 'mozilla::UniquePtr'?
        unique_ptr<FILE, int(*)(FILE*)> __h(__file_, fclose);
        ^
/Users/ehsan/moz/src/gfx/skia/skia/include/core/../private/SkUniquePtr.h:22:24: note: expanded from macro 'unique_ptr'
    #define unique_ptr UniquePtr
                       ^
/Users/ehsan/moz/src/obj-ff-clang-plugin.noindex/dist/include/mozilla/UniquePtr.h:154:7: note: 'mozilla::UniquePtr' declared here
class UniquePtr
      ^
In file included from /Users/ehsan/moz/src/gfx/2d/Factory.cpp:47:
In file included from /Users/ehsan/moz/src/gfx/2d/DrawTargetRecording.h:11:
In file included from /Users/ehsan/moz/src/gfx/2d/DrawEventRecorder.h:17:
In file included from /Users/ehsan/Downloads/clang+llvm-3.7.0-x86_64-apple-darwin/bin/../include/c++/v1/set:389:
/Users/ehsan/Downloads/clang+llvm-3.7.0-x86_64-apple-darwin/bin/../include/c++/v1/__tree:1043:13: error: no template named 'UniquePtr'; did you mean 'mozilla::UniquePtr'?
    typedef unique_ptr<__node, _Dp> __node_holder;
            ^
/Users/ehsan/moz/src/gfx/skia/skia/include/core/../private/SkUniquePtr.h:22:24: note: expanded from macro 'unique_ptr'
    #define unique_ptr UniquePtr
                       ^
/Users/ehsan/moz/src/obj-ff-clang-plugin.noindex/dist/include/mozilla/UniquePtr.h:154:7: note: 'mozilla::UniquePtr' declared here
class UniquePtr
      ^
In file included from /Users/ehsan/moz/src/gfx/2d/Factory.cpp:47:
In file included from /Users/ehsan/moz/src/gfx/2d/DrawTargetRecording.h:11:
In file included from /Users/ehsan/moz/src/gfx/2d/DrawEventRecorder.h:17:
In file included from /Users/ehsan/Downloads/clang+llvm-3.7.0-x86_64-apple-darwin/bin/../include/c++/v1/set:389:
/Users/ehsan/Downloads/clang+llvm-3.7.0-x86_64-apple-darwin/bin/../include/c++/v1/__tree:1721:9: error: no member named 'get_deleter' in
      'mozilla::UniquePtr<std::__1::__tree_node<const void *, void *>, std::__1::__tree_node_destructor<std::__1::allocator<std::__1::__tree_node<const void *, void *> > > >'
    __h.get_deleter().__value_constructed = true;
    ~~~ ^
/Users/ehsan/Downloads/clang+llvm-3.7.0-x86_64-apple-darwin/bin/../include/c++/v1/__tree:1793:25: note: in instantiation of function template specialization 'std::__1::__tree<const
      void *, std::__1::less<const void *>, std::__1::allocator<const void *> >::__construct_node<const void *>' requested here
    __node_holder __h = __construct_node(_VSTD::forward<_Vp>(__v));
                        ^
/Users/ehsan/Downloads/clang+llvm-3.7.0-x86_64-apple-darwin/bin/../include/c++/v1/set:603:25: note: in instantiation of function template specialization 'std::__1::__tree<const
      void *, std::__1::less<const void *>, std::__1::allocator<const void *> >::__insert_unique<const void *>' requested here
        {return __tree_.__insert_unique(_VSTD::move(__v));}
                        ^
/Users/ehsan/moz/src/gfx/2d/DrawEventRecorder.h:38:20: note: in instantiation of member function 'std::__1::set<const void *, std::__1::less<const void *>,
      std::__1::allocator<const void *> >::insert' requested here
    mStoredObjects.insert(aObject);
                   ^
In file included from /Users/ehsan/moz/src/gfx/2d/Factory.cpp:47:
In file included from /Users/ehsan/moz/src/gfx/2d/DrawTargetRecording.h:11:
In file included from /Users/ehsan/moz/src/gfx/2d/DrawEventRecorder.h:17:
In file included from /Users/ehsan/Downloads/clang+llvm-3.7.0-x86_64-apple-darwin/bin/../include/c++/v1/set:389:
/Users/ehsan/Downloads/clang+llvm-3.7.0-x86_64-apple-darwin/bin/../include/c++/v1/__tree:1721:9: error: no member named 'get_deleter' in
      'mozilla::UniquePtr<std::__1::__tree_node<unsigned long long, void *>, std::__1::__tree_node_destructor<std::__1::allocator<std::__1::__tree_node<unsigned long long, void *>
      > > >'
    __h.get_deleter().__value_constructed = true;
    ~~~ ^
/Users/ehsan/Downloads/clang+llvm-3.7.0-x86_64-apple-darwin/bin/../include/c++/v1/__tree:1861:29: note: in instantiation of function template specialization
      'std::__1::__tree<unsigned long long, std::__1::less<unsigned long long>, std::__1::allocator<unsigned long long> >::__construct_node<const unsigned long long &>' requested
      here
        __node_holder __h = __construct_node(__v);
                            ^
/Users/ehsan/Downloads/clang+llvm-3.7.0-x86_64-apple-darwin/bin/../include/c++/v1/set:599:25: note: in instantiation of member function 'std::__1::__tree<unsigned long long,
      std::__1::less<unsigned long long>, std::__1::allocator<unsigned long long> >::__insert_unique' requested here
        {return __tree_.__insert_unique(__v);}
                        ^
/Users/ehsan/moz/src/gfx/2d/DrawEventRecorder.h:50:21: note: in instantiation of member function 'std::__1::set<unsigned long long, std::__1::less<unsigned long long>,
      std::__1::allocator<unsigned long long> >::insert' requested here
    mStoredFontData.insert(aFontDataKey);
                    ^

Comment 1

[(no longer active)]

Attachment: Part 1: Import mozilla::Forward and mozilla::UniqePtr into the std namespace in a way that is compatible with libc++

Comment 2

[(no longer active)]

Attachment: Part 2: Rename UniquePtr::getDeleter() to get_deleter() in order to make it compatible with std::unique_ptr

Comment 3

[Lee Salzman [:lsalzman]]

Comment on attachment 8722025 [details] [diff] [review]
Part 1: Import mozilla::Forward and mozilla::UniqePtr into the std namespace in a way that is compatible with libc++

Review of attachment 8722025 [details] [diff] [review]:
-----------------------------------------------------------------

We were working on a slightly different long-term thinking way of working around this in bug 1248416 (by just natively using libc++ for Skia), but I think for now your proposed workaround is less dangerous than what we were trying to do in the short-term. I think this is advantageous to have this change in parallel.

Comment 4

[Lee Salzman [:lsalzman]]

Comment on attachment 8722025 [details] [diff] [review]
Part 1: Import mozilla::Forward and mozilla::UniqePtr into the std namespace in a way that is compatible with libc++

Review of attachment 8722025 [details] [diff] [review]:
-----------------------------------------------------------------

Oops, forgot r+.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8722026 [details] [diff] [review]
Part 2: Rename UniquePtr::getDeleter() to get_deleter() in order to make it compatible with std::unique_ptr

Review of attachment 8722026 [details] [diff] [review]:
-----------------------------------------------------------------

WFM, but see also bug 1248416. Feel free to split this patch out to a separate bug, if you want to just have things fixed up in the other bug.

Comment 6

[Lee Salzman [:lsalzman]]

(In reply to Lee Salzman [:lsalzman] from comment #4)
> Comment on attachment 8722025 [details] [diff] [review]
> Part 1: Import mozilla::Forward and mozilla::UniqePtr into the std namespace
> in a way that is compatible with libc++
> 
> Review of attachment 8722025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Oops, forgot r+.

One more small nitpick, so long as you're bringing UniquePtr into std, should probably bring DefaultDelete in too, just in case. Hopefully we won't need this for too much longer anyway.

Comment 7

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] from comment #5)
> WFM, but see also bug 1248416. Feel free to split this patch out to a
> separate bug, if you want to just have things fixed up in the other bug.

This looks useful even once we fix bug 1248416, since I think there's value in making UniquePtr a drop-in replacement for std::unique_ptr.  I'll land this here.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/390c755ec4b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/aab8109b4132

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/390c755ec4b3
https://hg.mozilla.org/mozilla-central/rev/aab8109b4132