Bug 153617 - Add a variadic function template for creating new RefCounted objects and use it instead of the numerous per-class create() functions
Summary: Add a variadic function template for creating new RefCounted objects and use ...
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Enhancement
Assignee: Andy Estes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-28 14:45 PST by Andy Estes
Modified: 2016-02-22 16:24 PST (History)
6 users (show)

See Also:


Attachments
Proof of concept (12.49 KB, patch)
2016-02-08 13:13 PST, Andy Estes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Estes 2016-01-28 14:45:41 PST
In https://bugs.webkit.org/show_bug.cgi?id=153612, DedicatedWorkerThread::create() was changed to be a variadic function template to remove duplication when adding arguments to DedicatedWorkerThread's constructor. It would be nice to generalize this to all RefCounted objects, so that we do not have to create a custom create() function for each RefCounted class, duplicating the constructor signature.
Comment 1 BJ Burg 2016-02-05 17:19:42 PST
This would be awesome.
Comment 2 Andy Estes 2016-02-08 13:13:38 PST
Created attachment 270876 [details]
Proof of concept

I took a stab at this over the weekend. Here's a proof of concept with a few examples of how this would be adopted.

I ended up adding a static create function in WTF::Ref. It's not possible to do this correctly in RefCounted, since the RefCounted template is not always instantiated with the class that needs to be constructed. I like the way this ended up looking, since it plays really nicely with auto, e.g.:

    auto stopwatch = Ref<Stopwatch>::create();

There are a few things I don't love about this approach, though:

1. Classes that wish to use this function need to become friends with WTF::Ref so that it can call their private/protected constructors. Making constructors public would defeat the point of this exercise, so this is probably an acceptable trade-off.
2. Some classes have non-trivial create functions, where extra work needs to be done before or after calling constructors, and some classes even return a null RefPtr in some cases. Those classes should be ineligible for using Ref::create() (even though they might choose to use that function internally). I'd like to add some enforcement at compile time that prevents callers from using Ref::create if a per-class create function exists. It might make sense to add a trait to classes that wish to retain a custom create function that Ref::create can static_assert() on.

I'd welcome any feedback on how to make this better!
Comment 3 BJ Burg 2016-02-20 21:56:28 PST
Comment on attachment 270876 [details]
Proof of concept

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

> Source/JavaScriptCore/API/JSProfilerPrivate.cpp:39
> +    auto stopwatch = Ref<Stopwatch>::create();

Does this mean that the caller could accidentally create a RefPtr<T> even if the custom T::create() would always return a Ref<T>?
Comment 4 BJ Burg 2016-02-20 22:00:46 PST
Comment on attachment 270876 [details]
Proof of concept

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

> Source/JavaScriptCore/API/JSWeakObjectMapRefInternal.h:52
> +    friend class Ref<OpaqueJSWeakObjectMap>;

Depending on the current tastes among reviewers, this could be a macro a la MAKE_FAST_ALLOCATED or whatever it is. It would definitely make the pervasive friend'ing more obvious if, say, it said ALLOW_CREATE_FUNCTION_FOR(OpaqueJSWeakObjectMap). Even better if the class name could be derived automatically, so the macro would be ALLOW_CREATE_FUNCTION or something similar.
Comment 5 Andy Estes 2016-02-22 16:23:38 PST
(In reply to comment #3)
> Comment on attachment 270876 [details]
> Proof of concept
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=270876&action=review
> 
> > Source/JavaScriptCore/API/JSProfilerPrivate.cpp:39
> > +    auto stopwatch = Ref<Stopwatch>::create();
> 
> Does this mean that the caller could accidentally create a RefPtr<T> even if
> the custom T::create() would always return a Ref<T>?

Yeah, I alluded to this issue in comment #2. I agree that a custom T::create() and Ref::create() should be mutually exclusive. We should have a mechanism that prevents you at compile time opting into Ref::create() if you have a custom create() functions.

This could be part of the macro you describe in your following comment.
Comment 6 Andy Estes 2016-02-22 16:24:53 PST
... and the same macro would also prevent you from adding a custom create() function once you've opted in to Ref::create().