Closed
Bug 684919
Opened 13 years ago
Closed 12 years ago
Add a template for storing a flag with an nsCOMPtr
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
INCOMPLETE
People
(Reporter: khuey, Assigned: khuey)
References
Details
Attachments
(1 file, 3 obsolete files)
8.63 KB,
patch
|
khuey
:
review+
benjamin
:
superreview+
|
Details | Diff | Splinter Review |
This adds mozilla::COMPtrAndFlag, which is basically a refcounted comptr version of js's AlignedPtrAndFlag.
Attachment #558527 -
Flags: review?(benjamin)
Assignee | ||
Comment 1•13 years ago
|
||
Joe thinks we should have an operator T* or something so that you don't need .Ptr() everywhere.
Comment 2•13 years ago
|
||
Comment on attachment 558527 [details] [diff] [review] Patch I'm going to delegate this review to jlebar.
Attachment #558527 -
Flags: review?(benjamin) → review?(justin.lebar+bug)
Comment 3•13 years ago
|
||
Comment on attachment 558527 [details] [diff] [review] Patch khuey says he's going to update this patch, so canceling the review for now.
Attachment #558527 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #558527 -
Attachment is obsolete: true
Attachment #560507 -
Flags: review?(justin.lebar+bug)
Comment 5•13 years ago
|
||
Comment on attachment 560507 [details] [diff] [review] paatch >+template <class T> >+class COMPtrAndFlag { I'd like a brief comment motivating why this class exists. >+ ~COMPtrAndFlag() >+ { >+ UnsetFlag(); >+ } Please explain in a comment why you have to unset the flag here. >+ nsCOMPtr<T> mCOMPtr; This needs a big warning next to it -- do not modify mCOMPtr without unsetting the flag first, because we'll crash and burn! This whole business with manually unsetting the flag and the RAII flag-saver class seems error-prone to me. Would it work to use a template? template<class PtrType> private void Set(PtrType aPtr, bool aFlag) and call that from all your overrides of Set and SetPtr? That way, you only assign to mCOMPtr from one place. >+template <class T> >+class COMPtrAndFlagGetterAddRefs >+ /* >+ ... >+ >+ This class is designed to be used for anonymous temporary objects in the >+ argument list of calls that return COM interface pointers, e.g., >+ >+ COMPtrAndFlag<IFoo> fooP; >+ ...->GetAddRefedPointer(getter_AddRefs(fooP)) >+ >+ DO NOT USE THIS TYPE DIRECTLY IN YOUR CODE. Use |getter_AddRefs()| instead. >+ >+ When initialized with a |COMPtrAndFlag|, as in the example above, it returns >+ a |void**|, a |T**|, or an |nsISupports**| as needed, that the >+ outer call (|GetAddRefedPointer| in this case) can fill in. >+ */ >+{ This is a strange place for a comment. Can you move it above the class name? Also, if I'm not supposed to use this type in my code, can you put it in a namespace? mozilla::you_really_shouldnt_use_this_namespace_in_your_code or something. >diff --git a/xpcom/tests/TestCOMPtr.cpp b/xpcom/tests/TestCOMPtr.cpp >--- a/xpcom/tests/TestCOMPtr.cpp >+++ b/xpcom/tests/TestCOMPtr.cpp >@@ -40,6 +40,7 @@ > #include <stdio.h> > #include "nsCOMPtr.h" > #include "nsISupports.h" >+#include "mozilla/PtrAndFlag.h" > > #define NS_IFOO_IID \ > { 0x6f7652e0, 0xee43, 0x11d1, \ >@@ -587,6 +588,43 @@ main() > AnISupportsPtrPtrContext( getter_AddRefs(supportsP) ); > } > >+ { >+ IBar* ibar1 = new IBar; >+ mozilla::COMPtrAndFlag<IFoo> foop( do_QueryInterface(ibar1) , false); >+ if (foop.Flag()) >+ return -1; >+ if (foop.Ptr() != ibar1) >+ return -1; >+ >+ foop.SetFlag(true); >+ if (!foop.Flag()) >+ return -1; >+ if (foop.Ptr() != ibar1) >+ return -1; >+ >+ IBar* ibar2 = new IBar; >+ mozilla::COMPtrAndFlag<IFoo> foop2( do_QueryInterface(ibar2) , true); >+ >+ if (!foop2.Flag()) >+ return -1; >+ if (foop2.Ptr() != ibar2) >+ return -1; >+ >+ foop2.SetFlag(false); >+ if (foop2.Flag()) >+ return -1; >+ if (foop2.Ptr() != ibar2) >+ return -1; >+ >+ } >+ >+ { >+ mozilla::COMPtrAndFlag<nsISupports> supportsP; >+ >+ AVoidPtrPtrContext( getter_AddRefs(supportsP) ); >+ AnISupportsPtrPtrContext( getter_AddRefs(supportsP) ); >+ } The first block is indented with tabs, but the second one is indented with spaces. But maybe that's to match the style of this file. :-P
Attachment #560507 -
Flags: review?(justin.lebar+bug) → review-
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #5) > This whole business with manually unsetting the flag and the RAII flag-saver > class seems error-prone to me. Would it work to use a template? > > template<class PtrType> > private void Set(PtrType aPtr, bool aFlag) > > and call that from all your overrides of Set and SetPtr? That way, you only > assign to mCOMPtr from one place. Yes, that's much nicer.
Assignee | ||
Comment 7•13 years ago
|
||
Attachment #560507 -
Attachment is obsolete: true
Attachment #562434 -
Flags: review?(justin.lebar+bug)
Comment 8•13 years ago
|
||
Comment on attachment 562434 [details] [diff] [review] Patch Would it be cleaner to get rid of the AutoFlagPersister now that it's only used in one place? Also, maybe the file should be COMPtrAndFlag.h?
Attachment #562434 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 9•13 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #8) > Comment on attachment 562434 [details] [diff] [review] [diff] [details] [review] > Patch > > Would it be cleaner to get rid of the AutoFlagPersister now that it's only > used in one place? Hmm, perhaps. > Also, maybe the file should be COMPtrAndFlag.h? I fully expect to want to add an nsRefPtr version eventually.
Assignee | ||
Comment 10•13 years ago
|
||
gcc complains about one template<class T> shadowing another, so this has a minor change to template<class U> on the friend decl. Carrying forward r=jlebar. This is the kind of thing that wants superreview I think ... so tagging bsmedberg again.
Attachment #562434 -
Attachment is obsolete: true
Attachment #562505 -
Flags: superreview?(benjamin)
Attachment #562505 -
Flags: review+
Comment 11•13 years ago
|
||
Comment on attachment 562505 [details] [diff] [review] Patch I don't like the file name. sr=me with the filename matching the class name, so mozilla/COMPtrAndFlag.h I'm bummed that COMPtrAndFlagGetterAddRefs can't be shared code, but I guess we'd be adding another level of template weirdness on top of this already weird code. Style nits: opening brace of a class should be in column 0, e.g. template<class T> class COMPtrAndFlag { Also can you add a comment near the top of both nsCOMPtr.h and this file saying that people should keep the visible API in sync?
Attachment #562505 -
Flags: superreview?(benjamin) → superreview+
Assignee | ||
Comment 12•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f2c16b13cf65
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Comment 13•13 years ago
|
||
Backed out for Android reftest failures: https://hg.mozilla.org/mozilla-central/rev/29c1738d7e27
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla10 → ---
Assignee | ||
Comment 14•12 years ago
|
||
My current patch stack does not end up using this. I'm going to mark this as INCOMPLETE, but we can resurrect it if someone needs it in the future.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 12 years ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•