Bug 214171 – Rename createBoxPtr into adoptBoxPtr

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Xabier Rodríguez Calvar]

Rename createBoxPtr into adoptBoxPtr

[Xabier Rodríguez Calvar]

Created attachment 403943 [details]
Patch

Address comments by Darin in bug 214144

[Xabier Rodríguez Calvar]

Created attachment 403968 [details]
Patch

I also changed a call to createBoxPtr that was already in trunk.

[Darin Adler]

Comment on attachment 403968 [details]
Patch

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

> Source/WTF/wtf/BoxPtr.h:49
> +template<typename T> BoxPtr<T> adoptBoxPtr(T* ptr)

I don’t think adoptBoxPtr is quite the right name. The return value is a BoxPtr, the thing being adopted is not a BoxPtr. Maybe the name needs both the words create (for the BoxPtr) and adopt (for the thing it’s taking)?

> Source/WTF/wtf/BoxPtr.h:51
>      return BoxPtr<T>::create(ptr);

I think Box::create also needs to be renamed for the same reason; as with this function, it takes ownership of a raw pointer and so needs a special name making that clear.

[Xabier Rodríguez Calvar]

(In reply to Darin Adler from comment #3)
> Comment on attachment 403968 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403968&action=review
> 
> > Source/WTF/wtf/BoxPtr.h:49
> > +template<typename T> BoxPtr<T> adoptBoxPtr(T* ptr)
> 
> I don’t think adoptBoxPtr is quite the right name. The return value is a
> BoxPtr, the thing being adopted is not a BoxPtr. Maybe the name needs both
> the words create (for the BoxPtr) and adopt (for the thing it’s taking)?

I guess it could be do something like adoptPointerIntoBoxPtr ? I cannot get a better name, but I guess we should iron it out before writing another patch, wdyt?

> > Source/WTF/wtf/BoxPtr.h:51
> >      return BoxPtr<T>::create(ptr);
> 
> I think Box::create also needs to be renamed for the same reason; as with
> this function, it takes ownership of a raw pointer and so needs a special
> name making that clear.

This create comes from Box class, something I don't think we should rename.

[Darin Adler]

(In reply to Xabier Rodríguez Calvar from comment #4)
> (In reply to Darin Adler from comment #3)
> > I think Box::create also needs to be renamed for the same reason; as with
> > this function, it takes ownership of a raw pointer and so needs a special
> > name making that clear.
> 
> This create comes from Box class, something I don't think we should rename.

Why? What makes the Box class immune to naming considerations?

[Xabier Rodríguez Calvar]

(In reply to Darin Adler from comment #5)
> (In reply to Xabier Rodríguez Calvar from comment #4)
> > (In reply to Darin Adler from comment #3)
> > > I think Box::create also needs to be renamed for the same reason; as with
> > > this function, it takes ownership of a raw pointer and so needs a special
> > > name making that clear.
> > 
> > This create comes from Box class, something I don't think we should rename.
> 
> Why? What makes the Box class immune to naming considerations?

Well, yes, we could, but wouldn't recommend it here since Box does create objects in place.

[Darin Adler]

(In reply to Xabier Rodríguez Calvar from comment #6)
> Well, yes, we could, but wouldn't recommend it here since Box does create
> objects in place.

Doesn’t that function take ownership of a pointer? How is that "creating an object in place"?

[Darin Adler]

(In reply to Darin Adler from comment #7)
> (In reply to Xabier Rodríguez Calvar from comment #6)
> > Well, yes, we could, but wouldn't recommend it here since Box does create
> > objects in place.
> 
> Doesn’t that function take ownership of a pointer? How is that "creating an
> object in place"?

Oh, OK, I guess not. My bad. I misunderstood.

[Xabier Rodríguez Calvar]

Created attachment 404453 [details]
Patch

Landing this tomorrow morning CEST.

[EWS]

Committed r264499: <https://trac.webkit.org/changeset/264499>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 404453 [details].

[Radar WebKit Bug Importer]

<rdar://problem/65707686>