Bug 238392

Created attachment 455799 [details]
Patch

Created attachment 455800 [details]
Patch

I've personally never been a fan of macros. Curious how other feel about this.
It does reduce boilerplate code.. (although I'd argue at the cost of readability).

I think this is similar to WTF_MAKE_ISO_ALLOCATED, WTF_MAKE_NONCOPYABLE, and WTF_MAKE_NONMOVABLE

(In reply to Alex Christensen from comment #4)
> I think this is similar to WTF_MAKE_ISO_ALLOCATED, WTF_MAKE_NONCOPYABLE, and
> WTF_MAKE_NONMOVABLE

There is one huge difference, the existing one to do define API I want to call. They add some boilerplate I don't really care about as an API user.

Your new macro though declare one of the most important APIs of the class. For new comers especially, it will make our API is little less clear as the way to construct an object of this class is no longer directly readable directly by viewing the class.

(In reply to Chris Dumez from comment #5)
> (In reply to Alex Christensen from comment #4)
> > I think this is similar to WTF_MAKE_ISO_ALLOCATED, WTF_MAKE_NONCOPYABLE, and
> > WTF_MAKE_NONMOVABLE
> 
> There is one huge difference, the existing one to do define API I want to
> call. They add some boilerplate I don't really care about as an API user.
> 
> Your new macro though declare one of the most important APIs of the class.
> For new comers especially, it will make our API is little less clear as the
> way to construct an object of this class is no longer directly readable
> directly by viewing the class.

Might be worse discussing a bit more broadly (maybe webkit-dev?). I am not objecting if people like it but I personally don't like the idea of using macros to define public API on our objects.

Comment on attachment 455800 [details]
Patch

I think I like adding this macro; I’m not too worried about hiding the create function from people reading the code.

I think the name is a little too short, WTF_CREATE doesn’t seem quite right. After all, this doesn’t create anything. It defines a function named create. Also, it’s only good for reference counted objects. A short name is neat, but this is too terse and vague, I think.

I don’t like using a semicolon after each use of the macro; that’s just an error.

It would be nicer if the use of this macro was always right next to the constructor, since the argument list of the constructor is an important part of seeing how to use the class; in the older idioms that is not so. You can see the arguments to create just by looking at create. That’s already suboptimal with these forwarding template create functions that we used for CSSRGB and the like. But to state the obvious we still want to make the constructor private and the create function public.

One downside of creating this macro, especially if it has a short name, is that it gives the impression that this is the only correct way to define a create function. And it’s not. There might be more work the create function has to do beyond just calling a constructor, and we might want a tryCreate function instead of a create function, or a create function that can fail. But this does do the job of making sure new objects are adopted into a Ref, making that standard idiom more foolproof, and I sure do like that.

<rdar://problem/91182646>

Comment on attachment 455800 [details]
Patch

I think I like Zan's approach in https://bugs.webkit.org/show_bug.cgi?id=238392 better.  As long as you have to write something in the class, it may as well be befriending RefCounted<T> instead of a macro.