Bug 212379 – [WTF] Implement new BoxPtr alias

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Xabier Rodríguez Calvar]

[WTF] Implement new BoxPtr class

[Xabier Rodríguez Calvar]

Created attachment 400253 [details]
Patch

[Xabier Rodríguez Calvar]

This class will be used in a further implementation of OpenCDM in the GStreamer based ports.

[Sam Weinig]

Hey Xabier, how do you see this differing from std::unique_ptr?

[Xabier Rodríguez Calvar]

(In reply to Sam Weinig from comment #3)
> Hey Xabier, how do you see this differing from std::unique_ptr?

This is shareable (it has copy constructor) and the pointer inside is deleted when when all refs are gone.

[Michael Catanzaro]

Comment on attachment 400253 [details]
Patch

I understand the purpose of BoxPtr is to add refcounting to classes without an internal reference count. So... I think you have reinvented std::shared_ptr, yes? :)

[Sam Weinig]

(In reply to Sam Weinig from comment #3)
> Hey Xabier, how do you see this differing from std::unique_ptr?

Oh, blergh, yeah. What I really should have asked is how this is different than WTF::Box<std::unique_ptr<T, Deleter>>?

[Xabier Rodríguez Calvar]

I think Michael is right. For some reason I remember somebody telling me that shared_ptr was not a good idea inside WebKit and as in the code base I usually moved I had never seen it, I didn't even bother to check if it was used.

I just moved the OpenCDM code from BoxPtr to shared_ptr and everything works as expected so I'll close this.

[Michael Catanzaro]

(In reply to Xabier Rodríguez Calvar from comment #7)
> I think Michael is right. For some reason I remember somebody telling me
> that shared_ptr was not a good idea inside WebKit and as in the code base I
> usually moved I had never seen it, I didn't even bother to check if it was
> used.

Well... shared_ptr is *usually* not a good idea, because we use RefPtr instead, for RefCounted classes. But when you have an external class provided by an external library that doesn't have an internal refcount, then RefPtr is not an option, and std::shared_ptr is a good solution. Currently I see one use in JSC and three in WTF, which is not much precedent for using shared_ptr in WebKit, but that's OK since it's the right tool for what you need to do.

It's a shame you wrote all those tests that will not be committed. :(

[Sam Weinig]

There is also the existing WTF::Box class, which should support this use case in many places.

[Xabier Rodríguez Calvar]

(In reply to Sam Weinig from comment #9)
> There is also the existing WTF::Box class, which should support this use
> case in many places.

Yes, but not in this one because Box is thought for objects that can be built in place and in this case the you get a "C object" created with a function and returned as a struct. I had actually named BoxPtr because it was a very similar use case but not needing to have the object in place.

Could I use Box<std::unique_ptr<OpenCDMSession>>? Probably yes, but IMHO it's quite a convolution if shared_ptr does the job.

[Konstantin Tokarev]

(In reply to Xabier Rodríguez Calvar from comment #10)
> Could I use Box<std::unique_ptr<OpenCDMSession>>? Probably yes, but IMHO
> it's quite a convolution if shared_ptr does the job.

FWIW, Box uses FastMalloc while shared_ptr doesn't, and we could add an alias for Box<std::unique_ptr<T>>

[Xabier Rodríguez Calvar]

(In reply to Konstantin Tokarev from comment #11)
> FWIW, Box uses FastMalloc while shared_ptr doesn't, and we could add an
> alias for Box<std::unique_ptr<T>>

Let's try this then.

[Xabier Rodríguez Calvar]

Created attachment 403057 [details]
Patch

[Konstantin Tokarev]

Comment on attachment 403057 [details]
Patch

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

> Source/WTF/wtf/BoxPtr.h:54
> +    return Box<std::unique_ptr<T, BoxPtrDeleter<T>>>::create(ptr);

This can be shortened to BoxPtr<T>::create(ptr)

[Darin Adler]

I, for one, would prefer that we use std::shared_ptr rather than making our own. But maybe we can’t figure out how to make shared_ptr use FastMalloc by default?

[Michael Catanzaro]

(In reply to Darin Adler from comment #15)
> I, for one, would prefer that we use std::shared_ptr rather than making our
> own. But maybe we can’t figure out how to make shared_ptr use FastMalloc by
> default?

We could create a ::create static factory function as usual that would call std::allocate_shared with an appropriate allocator. We would have to think about using FastMalloc when writing the ::create function, but not in all the dozens of places we might want to create new objects.

I don't think something like WTF_MAKE_FAST_ALLOCATED would be possible, though.

[Xabier Rodríguez Calvar]

Well, I'm going to go ahead with the patch that I had depending on this with std::shared_ptr. If we agree on somthing eventually, I'll change it later.

[Darin Adler]

Comment on attachment 403057 [details]
Patch

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

> Source/WTF/wtf/BoxPtr.h:36
> +    void operator()(T*) const { ASSERT_NOT_REACHED(); }

I think if we define this without a function body, we get something even better. A link time error rather than a runtime assertion. Maybe even a compile time error.

> Source/WTF/wtf/BoxPtr.h:39
> +#define WTF_DEFINE_BOXPTR_DELETER(typeName, deleterFunc)        \

In WebKit we don’t use abbreviations like “func“. Please call this function not func.

> Source/WTF/wtf/BoxPtr.h:58
> +bool operator==(const BoxPtr<T>& lhs, const BoxPtr<T>& rhs)

This is quite risky. Doing this for == and not !=, and also the effect this could have a generic code that really does need to do a pointer equality. I understand this is the heart of what you mean by using a pointer to box something, but I am quite concerned that this technique may not work well.

> Tools/TestWebKitAPI/Tests/WTF/BoxPtr.cpp:163
> +#if COMPILER(CLANG)

Compiler.h has macros for this kind of thing, and we should use those.

[Konstantin Tokarev]

Comment on attachment 403057 [details]
Patch

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

>> Source/WTF/wtf/BoxPtr.h:36
>> +    void operator()(T*) const { ASSERT_NOT_REACHED(); }
> 
> I think if we define this without a function body, we get something even better. A link time error rather than a runtime assertion. Maybe even a compile time error.

"void operator()(T*) const = delete" would make it compile error

[Xabier Rodríguez Calvar]

Created attachment 403705 [details]
Patch

Addressed all comments.

(In reply to Darin Adler from comment #18)
> > Source/WTF/wtf/BoxPtr.h:58
> > +bool operator==(const BoxPtr<T>& lhs, const BoxPtr<T>& rhs)
> 
> This is quite risky. Doing this for == and not !=, and also the effect this
> could have a generic code that really does need to do a pointer equality. I
> understand this is the heart of what you mean by using a pointer to box
> something, but I am quite concerned that this technique may not work well.

In my use case it helps, otherwise I would have to do the comparison by myself.

[Darin Adler]

(In reply to Xabier Rodríguez Calvar from comment #20)
> In my use case it helps, otherwise I would have to do the comparison by
> myself.

Obviously. My comment was about all the other repercussions. Leaving out != seems especially capricious.

[Konstantin Tokarev]

Comment on attachment 403705 [details]
Patch

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

> Source/WTF/wtf/BoxPtr.h:78
> +    return !(*lhs == *rhs);

I think it was meant to be implemented via operator== defined above, i.e.

    return !(lhs == rhs);

[Xabier Rodríguez Calvar]

(In reply to Konstantin Tokarev from comment #22)
> Comment on attachment 403705 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403705&action=review
> 
> > Source/WTF/wtf/BoxPtr.h:78
> > +    return !(*lhs == *rhs);
> 
> I think it was meant to be implemented via operator== defined above, i.e.
> 
>     return !(lhs == rhs);

Definitely. I guess I should have written tests for that as well and then I would realized...

[Xabier Rodríguez Calvar]

Created attachment 403778 [details]
Patch

(In reply to Xabier Rodríguez Calvar from comment #23)
> (In reply to Konstantin Tokarev from comment #22)
> > Comment on attachment 403705 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=403705&action=review
> > 
> > > Source/WTF/wtf/BoxPtr.h:78
> > > +    return !(*lhs == *rhs);
> > 
> > I think it was meant to be implemented via operator== defined above, i.e.
> > 
> >     return !(lhs == rhs);
> 
> Definitely. I guess I should have written tests for that as well and then I
> would realized...

And no, the test intent was there but the test was wrong so it was still passing. I added another test to test against empty BoxPtrs that was making former code crash. This works.

[Darin Adler]

Comment on attachment 403778 [details]
Patch

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

> Source/WTF/wtf/BoxPtr.h:35
> +template<typename T>
> +struct BoxPtrDeleter {

I personally like the style where template is on the same line. Applies to all the lines in this file.

> Source/WTF/wtf/BoxPtr.h:39
> +#define WTF_DEFINE_BOXPTR_DELETER(typeName, deleterFunction)        \

Seems like this is a little bit hard to use correctly, but let's try this for now. Also, I suggest just one space before the \ rather than different amounts on each line.

[Xabier Rodríguez Calvar]

Created attachment 403853 [details]
Patch

(In reply to Darin Adler from comment #25)
> Comment on attachment 403778 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403778&action=review
> 
> > Source/WTF/wtf/BoxPtr.h:35
> > +template<typename T>
> > +struct BoxPtrDeleter {
> 
> I personally like the style where template is on the same line. Applies to
> all the lines in this file.

Done.

> > Source/WTF/wtf/BoxPtr.h:39
> > +#define WTF_DEFINE_BOXPTR_DELETER(typeName, deleterFunction)        \
> 
> Seems like this is a little bit hard to use correctly, but let's try this
> for now. Also, I suggest just one space before the \ rather than different
> amounts on each line.

Done.

I also indented the compiler checks and removed some of the checks I was doing because they were redundant (as the macros already check if the warning is available or not) and because piling up BEGINs like that was creating compiler diagnostic contexts I was not removing.

[EWS]

Committed r264163: <https://trac.webkit.org/changeset/264163>

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

[Radar WebKit Bug Importer]

<rdar://problem/65263992>

[Darin Adler]

Comment on attachment 403853 [details]
Patch

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

> Source/WTF/wtf/BoxPtr.h:51
> +    return Box<std::unique_ptr<T, BoxPtrDeleter<T>>>::create(ptr);

I don’t understand why we didn’t write this instead:

    return BoxPtr<T>::create(ptr);

[Xabier Rodríguez Calvar]

(In reply to Darin Adler from comment #29)
> Comment on attachment 403853 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403853&action=review
> 
> > Source/WTF/wtf/BoxPtr.h:51
> > +    return Box<std::unique_ptr<T, BoxPtrDeleter<T>>>::create(ptr);
> 
> I don’t understand why we didn’t write this instead:
> 
>     return BoxPtr<T>::create(ptr);

To be fixed in bug 214144.