Closed
Bug 1114880
Opened 10 years ago
Closed 10 years ago
Annotate some strong and weak references in XPCOM
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
Details
Attachments
(1 file)
11.59 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8540536 -
Flags: review?(nfroyd)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → ehsan.akhgari
Updated•10 years ago
|
Summary: Annotate some strong and wweak references in XPCOM → Annotate some strong and weak references in XPCOM
Comment 2•10 years ago
|
||
Comment on attachment 8540536 [details] [diff] [review]
Annotate some strong and wweak references in XPCOM
Review of attachment 8540536 [details] [diff] [review]:
-----------------------------------------------------------------
Your commit message needs a little tweaking for |wweak|.
r=me with the comments addressed.
::: xpcom/glue/nsComponentManagerUtils.h
@@ +78,5 @@
> virtual nsresult NS_FASTCALL operator()(const nsIID&, void**) const;
>
> private:
> + nsCOMPtr<nsIFactory> mFactory;
> + nsISupports* MOZ_WEAK_REF mOuter;
Why nsCOMPtr for mFactory, but annotation for mOuter? I would think that annotation would be sufficient for mFactory, too.
::: xpcom/glue/nsHashKeys.h
@@ +384,5 @@
> }
> enum { ALLOW_MEMMOVE = true };
>
> protected:
> + T* MOZ_STRONG_REF mKey;
I don't think this is semantically correct. In a brief grep of these, it looks like somebody else is keeping mKey alive, not the hash table. So I would think that this would be MOZ_WEAK_REF, because somebody is keeping this alive for me?
Attachment #8540536 -
Flags: review?(nfroyd) → review+
Comment 3•10 years ago
|
||
Yeah, nsPtrHashKey is definitely weak.
Comment 4•10 years ago
|
||
Maybe we should have called them MOZ_OWNING_REF and MOZ_NON_OWNING_REF.
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> Your commit message needs a little tweaking for |wweak|.
Hah!
> ::: xpcom/glue/nsComponentManagerUtils.h
> @@ +78,5 @@
> > virtual nsresult NS_FASTCALL operator()(const nsIID&, void**) const;
> >
> > private:
> > + nsCOMPtr<nsIFactory> mFactory;
> > + nsISupports* MOZ_WEAK_REF mOuter;
>
> Why nsCOMPtr for mFactory, but annotation for mOuter? I would think that
> annotation would be sufficient for mFactory, too.
OK, sure.
> ::: xpcom/glue/nsHashKeys.h
> @@ +384,5 @@
> > }
> > enum { ALLOW_MEMMOVE = true };
> >
> > protected:
> > + T* MOZ_STRONG_REF mKey;
>
> I don't think this is semantically correct. In a brief grep of these, it
> looks like somebody else is keeping mKey alive, not the hash table. So I
> would think that this would be MOZ_WEAK_REF, because somebody is keeping
> this alive for me?
Oops, my bad. I added MOZ_WEAK_REF afterwards and probably forgot to adjust this annotation. Sorry!
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> Maybe we should have called them MOZ_OWNING_REF and MOZ_NON_OWNING_REF.
They do seem less ambiguous, I suppose. I can change them later (have tons of local patches applied right now...)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #4)
> > Maybe we should have called them MOZ_OWNING_REF and MOZ_NON_OWNING_REF.
>
> They do seem less ambiguous, I suppose. I can change them later (have tons
> of local patches applied right now...)
Done: https://hg.mozilla.org/integration/mozilla-inbound/rev/9e07239decd3
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b9094d876dc5
https://hg.mozilla.org/mozilla-central/rev/9e07239decd3
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•