Get rid of RefPtr, replace with std::optional<Ref>?
I recently worked on a patch where - because of the organic refactoring of the patch over its development - I ended up with a std::optional<Ref> instead of a RefPtr. A followup review after it had already landed pointed this out, and it got me to thinking: Does RefPtr do anything for us today that std::optional<Ref> doesn’t? I kind of like the idea of replacing RefPtr with std::optional<Ref>. It makes it explicitly clear what object is actually holding the reference, and completely removes some of the confusion of “when should I use Ref vs RefPtr?" Thoughts? Thanks, ~Brady
On Sep 1, 2017, at 9:30 AM, Brady Eidson <beidson@apple.com> wrote:
I recently worked on a patch where - because of the organic refactoring of the patch over its development - I ended up with a std::optional<Ref> instead of a RefPtr.
A followup review after it had already landed pointed this out, and it got me to thinking:
Does RefPtr do anything for us today that std::optional<Ref> doesn’t?
The obvious things would be: uses less storage space, has a shorter name.
I kind of like the idea of replacing RefPtr with std::optional<Ref>. It makes it explicitly clear what object is actually holding the reference, and completely removes some of the confusion of “when should I use Ref vs RefPtr?"
Thoughts?
Thanks, ~Brady _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Sep 1, 2017, at 9:46 AM, Maciej Stachowiak <mjs@apple.com> wrote:
Does RefPtr do anything for us today that std::optional<Ref> doesn’t?
The obvious things would be: uses less storage space
Grumble. If that’s true (which, thinking about it, of course it is true) this is pretty much a nonstarter. So… nevermind. Thanks, ~Brady
On Sep 1, 2017, at 10:07 AM, Brady Eidson <beidson@apple.com> wrote:
On Sep 1, 2017, at 9:46 AM, Maciej Stachowiak <mjs@apple.com> wrote:
Does RefPtr do anything for us today that std::optional<Ref> doesn’t?
The obvious things would be: uses less storage space
Grumble. If that’s true (which, thinking about it, of course it is true) this is pretty much a nonstarter. So… nevermind.
Even though I disagree with your proposal, I don’t think this is a good reason since we could create a template specialization for std::optional<Ref>. I think we could probably do that anyway since this type can arise through template instantiations (one place says std::optional<T> and something else sets T to Ref): https://bugs.webkit.org/show_bug.cgi?id=176228 <https://bugs.webkit.org/show_bug.cgi?id=176228> -Filip
On Sep 1, 2017, at 10:07 AM, Brady Eidson <beidson@apple.com> wrote:
On Sep 1, 2017, at 9:46 AM, Maciej Stachowiak <mjs@apple.com> wrote:
Does RefPtr do anything for us today that std::optional<Ref> doesn’t?
The obvious things would be: uses less storage space
Grumble. If that’s true (which, thinking about it, of course it is true) this is pretty much a nonstarter. So… nevermind.
It might be possible to make a specialization of std::optional<Ref> that avoids the overhead for a separate initialized bool. I'm not sure if it is wise to write specializations of standard library template classes though. Regards, Maciej
The standard wants you to specialize things in std sometimes, and not other times... I’m not sure here’s clear guidance here. :( On Fri, Sep 1, 2017 at 10:46 AM Maciej Stachowiak <mjs@apple.com> wrote:
On Sep 1, 2017, at 10:07 AM, Brady Eidson <beidson@apple.com> wrote:
On Sep 1, 2017, at 9:46 AM, Maciej Stachowiak <mjs@apple.com> wrote:
Does RefPtr do anything for us today that std::optional<Ref> doesn’t?
The obvious things would be: uses less storage space
Grumble. If that’s true (which, thinking about it, of course it is true) this is pretty much a nonstarter. So… nevermind.
It might be possible to make a specialization of std::optional<Ref> that avoids the overhead for a separate initialized bool. I'm not sure if it is wise to write specializations of standard library template classes though.
Regards, Maciej
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
I think std::optional<Ref<Type>> looks ugly. Also, unlike RefPtr<>, I do not think it is copyable. It is pretty neat to be able to capture a RefPtr<> by value in a lambda. Also, how do you convert it to a raw pointer? myOptionalRef.value_or(nullptr) would not work. Not sure there would be a nice way to do so. Finally, the storage space argument from Maciej is a good one. -- Chris Dumez
On Sep 1, 2017, at 9:46 AM, Maciej Stachowiak <mjs@apple.com> wrote:
On Sep 1, 2017, at 9:30 AM, Brady Eidson <beidson@apple.com> wrote:
I recently worked on a patch where - because of the organic refactoring of the patch over its development - I ended up with a std::optional<Ref> instead of a RefPtr.
A followup review after it had already landed pointed this out, and it got me to thinking:
Does RefPtr do anything for us today that std::optional<Ref> doesn’t?
The obvious things would be: uses less storage space, has a shorter name.
I kind of like the idea of replacing RefPtr with std::optional<Ref>. It makes it explicitly clear what object is actually holding the reference, and completely removes some of the confusion of “when should I use Ref vs RefPtr?"
Thoughts?
Thanks, ~Brady _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Sep 1, 2017, at 10:09 AM, Chris Dumez <cdumez@apple.com> wrote:
I think std::optional<Ref<Type>> looks ugly. Also, unlike RefPtr<>, I do not think it is copyable. It is pretty neat to be able to capture a RefPtr<> by value in a lambda. Also, how do you convert it to a raw pointer? myOptionalRef.value_or(nullptr) would not work. Not sure there would be a nice way to do so.
Finally, the storage space argument from Maciej is a good one.
We could create a specialization for std::optional<Ref>. Filed: https://bugs.webkit.org/show_bug.cgi?id=176228 That seems like a good idea separately from whether it should be used instead of RefPtr. Even if we did have style prohibiting it, we might end up with such a type because of template specialization. I can see cases were std::optional<Ref> works more naturally into the surrounding code than RefPtr. That probably happens if your code is already based on Ref. In my experience there’s a lot of inertia to these things - once some code uses RefPtr enough, it can be awkward to introduce Ref and perhaps vice versa. I don’t find it very hard to switch between thinking in terms of Ref and RefPtr, so I don’t mind that our code uses both. I wouldn’t agree with a style that encourages using std::optional<Ref> instead of RefPtr, but I also wouldn’t want to disallow it. -Filip
-- Chris Dumez
On Sep 1, 2017, at 9:46 AM, Maciej Stachowiak <mjs@apple.com <mailto:mjs@apple.com>> wrote:
On Sep 1, 2017, at 9:30 AM, Brady Eidson <beidson@apple.com <mailto:beidson@apple.com>> wrote:
I recently worked on a patch where - because of the organic refactoring of the patch over its development - I ended up with a std::optional<Ref> instead of a RefPtr.
A followup review after it had already landed pointed this out, and it got me to thinking:
Does RefPtr do anything for us today that std::optional<Ref> doesn’t?
The obvious things would be: uses less storage space, has a shorter name.
I kind of like the idea of replacing RefPtr with std::optional<Ref>. It makes it explicitly clear what object is actually holding the reference, and completely removes some of the confusion of “when should I use Ref vs RefPtr?"
Thoughts?
Thanks, ~Brady _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org> https://lists.webkit.org/mailman/listinfo/webkit-dev
_______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
On Sep 1, 2017, at 9:30 AM, Brady Eidson <beidson@apple.com> wrote:
I recently worked on a patch where - because of the organic refactoring of the patch over its development - I ended up with a std::optional<Ref> instead of a RefPtr.
A followup review after it had already landed pointed this out, and it got me to thinking:
Does RefPtr do anything for us today that std::optional<Ref> doesn’t?
Simpler syntax.
I kind of like the idea of replacing RefPtr with std::optional<Ref>. It makes it explicitly clear what object is actually holding the reference, and completely removes some of the confusion of “when should I use Ref vs RefPtr?”
There are many places in JSC where we don’t even consider using Ref. -Filip
Thoughts?
Thanks, ~Brady _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
participants (5)
-
Brady Eidson
-
Chris Dumez
-
Filip Pizlo
-
JF Bastien
-
Maciej Stachowiak