Closed
Bug 1044658
Opened 10 years ago
Closed 10 years ago
WeakPtr: simplifications, allowing const-correct WeakPtr-to-const, and removing asWeakPtr()
Categories
(Core :: MFBT, defect)
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: bjacob, Assigned: bjacob)
References
Details
Attachments
(6 files, 1 obsolete file)
3.63 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
3.21 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
2.52 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
14.49 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
4.63 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
1.18 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Attachment #8463157 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8463158 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8463160 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8463161 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8463162 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8463167 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 7•10 years ago
|
||
Note: I'm using this in Bug 1044668.
Assignee | ||
Updated•10 years ago
|
Attachment #8463157 -
Flags: review?(jwalden+bmo) → review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Attachment #8463158 -
Flags: review?(jwalden+bmo) → review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Attachment #8463160 -
Flags: review?(jwalden+bmo) → review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Attachment #8463161 -
Flags: review?(jwalden+bmo) → review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Attachment #8463162 -
Flags: review?(jwalden+bmo) → review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Attachment #8463167 -
Flags: review?(jwalden+bmo) → review?(nfroyd)
Comment 8•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8463158 -
Flags: review?(nfroyd) → review+
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8463162 -
Flags: review?(nfroyd) → review+
Updated•10 years ago
|
Attachment #8463167 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 11•10 years ago
|
||
(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).
Assignee | ||
Comment 12•10 years ago
|
||
(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.
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8463160 -
Attachment is obsolete: true
Attachment #8464888 -
Flags: review?(nfroyd)
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Assignee | ||
Comment 15•10 years ago
|
||
Updated•10 years ago
|
Attachment #8464888 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 16•10 years ago
|
||
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
Comment 17•10 years ago
|
||
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•