Bug 124866 – VS2010 doesn't like std::make_unique

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Alex Christensen]

I'm not sure what the status of the std::make_unique standard is, or what it was in 2010, but its use in TiledBackingStore.h causes compile errors in VS2010.  Let's use nullptr instead.

[Alex Christensen]

Created attachment 217837 [details]
Patch

[Sam Weinig]

What compile error does it cause? It may just be the missing #include of <wtf/StdLibExtras.h>

[Alex Christensen]

(In reply to comment #2)
> What compile error does it cause? It may just be the missing #include of <wtf/StdLibExtras.h>
Even with that included, it makes this error many, many times:
8>c:\cygwin\webkit\source\webcore\platform\graphics\TiledBackingStore.h(45): error C2780: '_Unique_if<T>::_Unknown_bound std::make_unique(size_t)' : expects 1 arguments - 0 provided
8>          C:\cygwin\webkit\WebKitBuild\Debug_WinCairo\include\private\wtf/StdLibExtras.h(412) : see declaration of 'std::make_unique'

My solution fixes it and doesn't seem to break anything on any other systems.

[Sam Weinig]

(In reply to comment #3)
> (In reply to comment #2)
> > What compile error does it cause? It may just be the missing #include of <wtf/StdLibExtras.h>
> Even with that included, it makes this error many, many times:
> 8>c:\cygwin\webkit\source\webcore\platform\graphics\TiledBackingStore.h(45): error C2780: '_Unique_if<T>::_Unknown_bound std::make_unique(size_t)' : expects 1 arguments - 0 provided
> 8>          C:\cygwin\webkit\WebKitBuild\Debug_WinCairo\include\private\wtf/StdLibExtras.h(412) : see declaration of 'std::make_unique'
> 
> My solution fixes it and doesn't seem to break anything on any other systems.

But it changes the semantics. 

std::make_unique<TiledBackingStoreBackend>() is the equivalent of (new TiledBackingStoreBackend());
nullptr is the equivalent of NULL.

So the old code always had a non-null TiledBackingStoreBackend and now it will. 

A better work around would to be to stop using a default value for the parameter and just have two constructors.

1) TiledBackingStore(TiledBackingStoreClient*); 
2) TiledBackingStore(TiledBackingStoreClient*, std::unique_ptr<TiledBackingStoreBackend>);

Just make sure that in #1's implementation you create the backend.

[Alex Christensen]

Created attachment 219384 [details]
Patch

[Alex Christensen]

Created attachment 219386 [details]
Patch

[Darin Adler]

Comment on attachment 219386 [details]
Patch

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

> Source/WebCore/platform/graphics/TiledBackingStore.h:44
> -    TiledBackingStore(TiledBackingStoreClient*, std::unique_ptr<TiledBackingStoreBackend> = std::make_unique<TiledBackingStoreBackend>());
> +    TiledBackingStore(TiledBackingStoreClient*, std::unique_ptr<TiledBackingStoreBackend> = std::unique_ptr<TiledBackingStoreBackend>(new TiledBackingStoreBackend()));

Please don’t do this. Is there some other solution? The reason we use make_unique is so that we can search the entire project for "new" to find mistakes. We really don’t want to do this just to accommodate a broken compiler.

I suggest we use overloading instead:

    TiledBackingStore(TiledBackingStoreClient*);
    TiledBackingStore(TiledBackingStoreClient*, std::unique_ptr<TiledBackingStoreBackend>);

Then the make_unique can go into the TiledBackingStore.cpp file. I’m hoping we can get it to compile there.

By the way, this overloading also makes it clear that the single-non-default-argument constructor needs “explicit” added to it. Please do that.

[Darin Adler]

Comment on attachment 219386 [details]
Patch

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

>> Source/WebCore/platform/graphics/TiledBackingStore.h:44
>> +    TiledBackingStore(TiledBackingStoreClient*, std::unique_ptr<TiledBackingStoreBackend> = std::unique_ptr<TiledBackingStoreBackend>(new TiledBackingStoreBackend()));
> 
> Please don’t do this. Is there some other solution? The reason we use make_unique is so that we can search the entire project for "new" to find mistakes. We really don’t want to do this just to accommodate a broken compiler.
> 
> I suggest we use overloading instead:
> 
>     TiledBackingStore(TiledBackingStoreClient*);
>     TiledBackingStore(TiledBackingStoreClient*, std::unique_ptr<TiledBackingStoreBackend>);
> 
> Then the make_unique can go into the TiledBackingStore.cpp file. I’m hoping we can get it to compile there.
> 
> By the way, this overloading also makes it clear that the single-non-default-argument constructor needs “explicit” added to it. Please do that.

I see now that Sam suggested the same thing, but for some reason you did not do what he suggested.

[Alex Christensen]

> I see now that Sam suggested the same thing, but for some reason you did not do what he suggested.

I was trying to minimize changes, and I though I would have had to use the new operator in the implementation of TiledBackingStore(TiledBackingStoreClient*) anyway.  I tested it, and std::make_unique works in the implementation, but not as the default parameter.  I'll upload a better patch.  Sorry for the confusion.

[Alex Christensen]

Created attachment 219428 [details]
Patch

[Alex Christensen]

I was also trying to avoid the duplicate code that this adds.  Less duplicate code is always better, but fewer calls to new is also better.

[WebKit Commit Bot]

Comment on attachment 219428 [details]
Patch

Clearing flags on attachment: 219428

Committed r160843: <http://trac.webkit.org/changeset/160843>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.