WeakPtr: simplifications, allowing const-correct WeakPtr-to-const, and removing asWeakPtr()

RESOLVED FIXED in mozilla34

Status

()

Core
MFBT
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Other Branch
mozilla34
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

WeakPtr used to have an attempt at a thread-safe variant, which was removed as it didn't work (bug 923554). Now, the polymorphism that had been added to it (bug 863884) to make it possible for it to have a threadsafe variant, is left as useless complexity; while working on the below, I found that my quickest path was to remove that complexity and work on the simplified code. So I think we should just remove it. That is the purpose of the first two patches here: one removes the unused base classes, and one removed the unused template parameters.

The third patch here makes WeakPtr<const T> just work. The problem was that asWeakPtr() didn't have a const-qualified variant returning a WeakPtr<const T>, so calling asWeakPtr() on a const object just returned a WeakPtr<T>, which couldn't be assigned to a WeaKPtr<const T>.

The last two patches here focus on removing asWeakPtr() altogether. asWeakPtr() does useful, needed work: the first time that it is called, it allocates the WeakReference object. But I think that it should happen behind-the-scenes, automatically. If the intent was to make it explicit that nontrivial work including a heap allocation was happening, then that was not achieved by the "asWeakPtr" method name anyway. So once we agree that making nontrivial work explicit wasn't a goal in the first place, then I don't see any reason not to just make it implicit when assigning a T* to a WeakPtr<T>. Benefits include: 1) simpler syntax on the caller side, we generally design pointer classes so as to be syntactically similar to plain pointers, and this change makes WeakPtr more aligned to both plain pointers and our existing smart pointer classes (nsRefPtr, RefPtr, UniquePtr); and 2) removing asWeakPtr() removes the need to think of a better name for it, given that asWeakPtr was a bad name (for one thing, it transforms a _reference_ into a _pointer_, and the 'as' part wrongly suggests that it's just some kind of cast or referencing).
(Assignee)

Comment 1

3 years ago
Created attachment 8463157 [details] [diff] [review]
Patch 1: remove useless template parameters around WeakPtr
Attachment #8463157 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

3 years ago
Created attachment 8463158 [details] [diff] [review]
Patch 2: remove useless base classes around WeakPtr
Attachment #8463158 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

3 years ago
Created attachment 8463160 [details] [diff] [review]
Patch 3: Make WeakPtr<const T> work
Attachment #8463160 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

3 years ago
Created attachment 8463161 [details] [diff] [review]
Patch 4: Remove the need for asWeakPtr, and make asWeakPtr just return 'this'
Attachment #8463161 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

3 years ago
Created attachment 8463162 [details] [diff] [review]
Patch 5: remove asWeakPtr
Attachment #8463162 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 6

3 years ago
Created attachment 8463167 [details] [diff] [review]
Patch 6: Expand TestWeakPtr
Attachment #8463167 - Flags: review?(jwalden+bmo)
(Assignee)

Updated

3 years ago
Blocks: 1044668
(Assignee)

Comment 7

3 years ago
Note: I'm using this in Bug 1044668.
(Assignee)

Updated

3 years ago
Attachment #8463157 - Flags: review?(jwalden+bmo) → review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8463158 - Flags: review?(jwalden+bmo) → review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8463160 - Flags: review?(jwalden+bmo) → review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8463161 - Flags: review?(jwalden+bmo) → review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8463162 - Flags: review?(jwalden+bmo) → review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8463167 - Flags: review?(jwalden+bmo) → review?(nfroyd)
Comment on attachment 8463157 [details] [diff] [review]
Patch 1: remove useless template parameters around WeakPtr

Review of attachment 8463157 [details] [diff] [review]:
-----------------------------------------------------------------

If you want to make things slightly tidier, you could:

typedef detail::WeakReference<T> WeakReference;

in appropriate places.
Attachment #8463157 - Flags: review?(nfroyd) → review+
Attachment #8463158 - Flags: review?(nfroyd) → review+
Comment on attachment 8463160 [details] [diff] [review]
Patch 3: Make WeakPtr<const T> work

Review of attachment 8463160 [details] [diff] [review]:
-----------------------------------------------------------------

So, I can buy the asWeakPtr bit here, but I have questions about the other bits...

::: mfbt/WeakPtr.h
@@ +91,5 @@
>    explicit WeakReference(T* p) : mPtr(p) {}
>  
> +  template<typename U>
> +  explicit WeakReference(const WeakReference<U>& other)
> +    : mPtr(other.mPtr)

What is this doing in this patch?

@@ +170,5 @@
>  template <typename T>
>  class WeakPtr
>  {
>  public:
> +  WeakPtr(const WeakPtr& aOther)

I guess this is cleanup from one of the previous two patches?
Attachment #8463160 - Flags: review?(nfroyd) → feedback+
Comment on attachment 8463161 [details] [diff] [review]
Patch 4: Remove the need for asWeakPtr, and make asWeakPtr just return 'this'

Review of attachment 8463161 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but I'm interested on hearing your response to the below.

Also, not your fault, but with static_cast and reinterpret_cast being slung around, it'd be great if there were some static_asserts for some of these classes verifying that they were sizeof(T*).  Followup bug?

::: mfbt/WeakPtr.h
@@ +194,5 @@
> +  }
> +
> +  WeakPtr& operator=(T* aOther)
> +  {
> +    return *this = aOther->WeakPtrToSelf();

This method seems weirdly named; I realize that it's meant to be "get the weak pointer to myself", but I initially read it as "convert this thing--which is a weak pointer--to self"...which is nonsensical.

Maybe SelfReferencingWeakPtr is a better name?  WDYT about this concern?
Attachment #8463161 - Flags: review?(nfroyd) → review+
Attachment #8463162 - Flags: review?(nfroyd) → review+
Attachment #8463167 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #8)
> Comment on attachment 8463157 [details] [diff] [review]
> Patch 1: remove useless template parameters around WeakPtr
> 
> Review of attachment 8463157 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> If you want to make things slightly tidier, you could:
> 
> typedef detail::WeakReference<T> WeakReference;
> 
> in appropriate places.

Good idea, I've done that in patch 4 (so as to not have to rebase patch 2).
(In reply to Nathan Froyd (:froydnj) from comment #9)
> Comment on attachment 8463160 [details] [diff] [review]
> Patch 3: Make WeakPtr<const T> work
> 
> Review of attachment 8463160 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> So, I can buy the asWeakPtr bit here, but I have questions about the other
> bits...
> 
> ::: mfbt/WeakPtr.h
> @@ +91,5 @@
> >    explicit WeakReference(T* p) : mPtr(p) {}
> >  
> > +  template<typename U>
> > +  explicit WeakReference(const WeakReference<U>& other)
> > +    : mPtr(other.mPtr)
> 
> What is this doing in this patch?

Good catch! This was indeed totally useless. Remnant from some earlier attempts.

> 
> @@ +170,5 @@
> >  template <typename T>
> >  class WeakPtr
> >  {
> >  public:
> > +  WeakPtr(const WeakPtr& aOther)
> 
> I guess this is cleanup from one of the previous two patches?

This is indeed unrelated, gratuitously thrown into this patch. It's a relatively little known c++ fact that inside templated class bodies, when referencing the class being defined, it is useless to repeat the template parameters i.e. WeakPtr<T> can be simplified into WeakPtr.
Created attachment 8464888 [details] [diff] [review]
Patch 3: Make WeakPtr<const T> work
Attachment #8463160 - Attachment is obsolete: true
Attachment #8464888 - Flags: review?(nfroyd)
(In reply to Nathan Froyd (:froydnj) from comment #10)
> Comment on attachment 8463161 [details] [diff] [review]
> Patch 4: Remove the need for asWeakPtr, and make asWeakPtr just return 'this'
> 
> Review of attachment 8463161 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, but I'm interested on hearing your response to the below.
> 
> Also, not your fault, but with static_cast and reinterpret_cast being slung
> around, it'd be great if there were some static_asserts for some of these
> classes verifying that they were sizeof(T*).  Followup bug?

I'm not a fan of the reinterpret_cast either but I didn't find a very meaningful static assertion to add to make it less scary. Please feel free to file a followup bug if you can think of a definite improvement to make around here.

> 
> ::: mfbt/WeakPtr.h
> @@ +194,5 @@
> > +  }
> > +
> > +  WeakPtr& operator=(T* aOther)
> > +  {
> > +    return *this = aOther->WeakPtrToSelf();
> 
> This method seems weirdly named; I realize that it's meant to be "get the
> weak pointer to myself", but I initially read it as "convert this
> thing--which is a weak pointer--to self"...which is nonsensical.
> 
> Maybe SelfReferencingWeakPtr is a better name?  WDYT about this concern?

Good idea, done.

That concern didn't occur to me, but I do understand that 'To' is ambiguous and suggests a conversion, which is misleading here. Funny how the conciseness and flexibility that makes English a great language for programming is also what makes it so prone to this kind of ambiguity.
https://tbpl.mozilla.org/?tree=Try&rev=bba37e02bb45
Attachment #8464888 - Flags: review?(nfroyd) → review+
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd926197c6c9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f57c397a04
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/77a2a058bc91
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5beea07a9f5f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8a7cbe9ed925
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f26c915b1dcc
Assignee: nobody → bjacob
https://hg.mozilla.org/mozilla-central/rev/cd926197c6c9
https://hg.mozilla.org/mozilla-central/rev/a8f57c397a04
https://hg.mozilla.org/mozilla-central/rev/77a2a058bc91
https://hg.mozilla.org/mozilla-central/rev/5beea07a9f5f
https://hg.mozilla.org/mozilla-central/rev/8a7cbe9ed925
https://hg.mozilla.org/mozilla-central/rev/f26c915b1dcc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.