684919 - Add a template for storing a flag with an nsCOMPtr

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This adds mozilla::COMPtrAndFlag, which is basically a refcounted comptr version of js's AlignedPtrAndFlag.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Joe thinks we should have an operator T* or something so that you don't need .Ptr() everywhere.

Comment 2

[Benjamin Smedberg]

Comment on attachment 558527 [details] [diff] [review]
Patch

I'm going to delegate this review to jlebar.

Comment 3

[Justin Lebar (not reading bugmail)]

Comment on attachment 558527 [details] [diff] [review]
Patch

khuey says he's going to update this patch, so canceling the review for now.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: paatch

Comment 5

[Justin Lebar (not reading bugmail)]

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

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 8

[Justin Lebar (not reading bugmail)]

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?

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

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.

Comment 11

[Benjamin Smedberg]

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?

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/f2c16b13cf65

Comment 13

[Matt Brubeck (:mbrubeck)]

Backed out for Android reftest failures:
https://hg.mozilla.org/mozilla-central/rev/29c1738d7e27

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.