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.
This would be awesome.
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 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 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.
(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.
... and the same macro would also prevent you from adding a custom create() function once you've opted in to Ref::create().