Closed Bug 792954 Opened 8 years ago Closed 7 years ago

Add a WeakPtr implementation to use instead of nsISupportsWeakReference

Categories

(Core :: MFBT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

Details

Attachments

(1 file, 2 obsolete files)

This patch also replaces the usage of nsISupportsWeakReference in RasterImage as an example.
Attachment #663104 - Flags: review?(jwalden+bmo)
Attachment #663104 - Flags: review?(joe)
Attachment #663104 - Flags: review?(ehsan)
This time with the test
Attachment #663104 - Attachment is obsolete: true
Attachment #663104 - Flags: review?(jwalden+bmo)
Attachment #663104 - Flags: review?(joe)
Attachment #663104 - Flags: review?(ehsan)
Attachment #663108 - Flags: review?(jwalden+bmo)
Attachment #663108 - Flags: review?(joe)
Attachment #663108 - Flags: review?(ehsan)
Comment on attachment 663108 [details] [diff] [review]
Add a WeakPtr implementation to use instead of nsISupportsWeakReference

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

::: mfbt/WeakPtr.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> +   This class lets you can a pointer to an object 'Foo' without affecting it's

its.

@@ +76,5 @@
> +    // This can live beyond the lifetime of the class derived from SupportsWeakPtr
> +    class WeakReference : public RefCounted<WeakReference> {
> +      public:
> +        WeakReference(T* ptr) : ptr(ptr) {}
> +        T* get() {

Nit: please make the ctor explicit, and make get() const.

::: mfbt/tests/TestWeakPtr.cpp
@@ +12,5 @@
> +
> +int main()
> +{
> +  A* a = new A;
> +  A* a2 = new A;

I don't see where a2 is being used.

::: xpcom/glue/nsWeakReference.h
@@ +9,4 @@
>  
>  // nsWeakReference.h
>  
> +// See mfbt/WeakPtr.h for a more modern C++ implementation of weak references

s/modern/typesafe/.  Also it would be good to recommend people using WeakPtr when they don't need the thing to be exposed to JS.
Attachment #663108 - Flags: review?(ehsan) → review+
Comment on attachment 663108 [details] [diff] [review]
Add a WeakPtr implementation to use instead of nsISupportsWeakReference

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

::: mfbt/WeakPtr.h
@@ +60,5 @@
> +      }
> +      return WeakPtr<T>(weakRef);
> +    }
> +
> +    ~SupportsWeakPtr() {

Should SupportsWeakPtr's ctor and dtor be protected? SupportsWeakPtr will only ever be used as a base class.

@@ +61,5 @@
> +      return WeakPtr<T>(weakRef);
> +    }
> +
> +    ~SupportsWeakPtr() {
> +      //XXX: the reason for the !! and the paranthesis is not known.

s/paranthesis/parentheses/
Comment on attachment 663108 [details] [diff] [review]
Add a WeakPtr implementation to use instead of nsISupportsWeakReference

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

::: image/src/RasterImage.h
@@ +690,4 @@
>  class imgDecodeRequestor : public nsRunnable
>  {
>    public:
> +    imgDecodeRequestor(RasterImage &aContainer) {

& with type
Attachment #663108 - Flags: review?(joe) → review+
Comment on attachment 663108 [details] [diff] [review]
Add a WeakPtr implementation to use instead of nsISupportsWeakReference

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

Mostly a bunch of little-ish nits and stuff, cumulatively enough to want a second look.  I promise to get to it faster the second time!  Poke me on IRC when you post v2 and I'll look immediately.

The existing XPCOM weak-ref stuff doesn't have operator->, operator*, and all those things on it -- it just lets you get a strong pointer, then you use that til you're done.  That has the virtue of no double-indirection, but I guess it requires allocator knowledge (nsISupports-ness) that this deliberately eschews.  We may want to circle back here and add something like that if SupportsWeakPtr is parametrized with a RefCounted<T>, maybe.

::: mfbt/WeakPtr.h
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> +   This class lets you can a pointer to an object 'Foo' without affecting it's

Use this style for big documentation comments:

/**
 * line one...
 * line two...
 */

"This class" could refer to a number of classes here.  I would clarify this by specifiying SupportsWeakPtr<T> and WeakPtr<T> directly.

@@ +10,5 @@
> +   clear the pointer in the WeakReference without having to know about all of the
> +   WeakPtrs and allows the WeakReference to live beyond the lifetime of 'Foo'.
> +
> +   The overhead of WeakPtr is that accesses to 'Foo' becomes an additional dereference,
> +   and an additional heap allocated pointer sized object shared between all of the

heap-allocated pointer-sized

@@ +28,5 @@
> +   Then make sure you test ptr before you use it because it could
> +   have gone null in the mean time.
> +
> +   if (a)
> +      a->a = 5;

This example is a little bit disjoint to read, partly because the code is at the same indentation level as the comments around it (code should be indented two spaces), partly because the name "a" is used in multiple ways, partly because it's not quite showing an entire example at once.  Also, weak pointers don't make much sense for stack-allocated objects, as far as I can tell.  How does this strike you instead?

  // To have a class C support weak pointers, inherit from SupportsWeakPtr<C>.
  class C : public SupportsWeakPtr<C>
  {
    public:
      int num;
      void act();
  };

  C* ptr =  new C();

  // Get weak pointers to ptr.
  WeakPtr<C> weak = ptr->asWeakPtr();
  WeakPtr<C> other = ptr->asWeakPtr();

  // Test a weak pointer for validity before using it.
  if (weak) {
    weak->num = 17;
    weak->act();
  }

  // Destroying the underlying object clears weak pointers to it.
  delete ptr;

  MOZ_ASSERT(!weak, "Deleting |ptr| clears weak pointers to it.");
  MOZ_ASSERT(!other, "Deleting |ptr| clears all weak pointers to it.");

@@ +31,5 @@
> +   if (a)
> +      a->a = 5;
> +
> +   In comparison to nsISupportsWeakReference this provides typesafe
> +   WeakPtrs and does not require the users to be XPCOM classes.

mfbt is independent of XPCOM, so this shouldn't mention XPCOM.  I would instead phrase it this way: "WeakPtr is typesafe and may be used with any class.  It is not required that the class be reference-counted or allocated in any particular way."

@@ +33,5 @@
> +
> +   In comparison to nsISupportsWeakReference this provides typesafe
> +   WeakPtrs and does not require the users to be XPCOM classes.
> +
> +   The api was loosely inspired by Chromium's src/base/memory/weak_ptr.h

"API", and use http://src.chromium.org/svn/trunk/src/base/memory/weak_ptr.h instead so it's an easily-followed link.

@@ +42,5 @@
> +#define mozilla_WeakPtr_h_
> +
> +#include "mozilla/RefPtr.h"
> +#include "mozilla/TypeTraits.h"
> +#include "mozilla/Assertions.h"

Alphabetize these, and add mozilla/NullPtr.h since you're using it directly here.

@@ +49,5 @@
> +
> +template <typename T> class WeakPtr;
> +
> +template <typename T>
> +class SupportsWeakPtr {

class SupportsWeakPtr
{

@@ +54,5 @@
> +  public:
> +    WeakPtr<T> asWeakPtr() {
> +      if (!weakRef) {
> +        // We need to convert to up to T* otherwise we end up
> +        // with a SupportsWeakPtr<T>

"up to T*, or else we end with a SupportsWeakPtr<T>."

Although if it were me, I think I'd just remove the comment.  Upward static_cast<>s are inherently necessary with CRTP, so seeing one here shouldn't throw anyone off *too* much.  (And if it does, this isn't the place to teach them about CRTP.)

@@ +60,5 @@
> +      }
> +      return WeakPtr<T>(weakRef);
> +    }
> +
> +    ~SupportsWeakPtr() {

Yeah, protecting ctor/dtor sounds good, principle of least privilege and all.

@@ +61,5 @@
> +      return WeakPtr<T>(weakRef);
> +    }
> +
> +    ~SupportsWeakPtr() {
> +      //XXX: the reason for the !! and the paranthesis is not known.

The reason is that MOZ_STATIC_ASSERT is a macro, and macros aren't aware of C++ template parameters, so when you have FOO(A<1, 2, 3>) it looks like a macro FOO called with arguments |A<1|, |2|, |3>|.  Ergo, parentheses needed.  Yeah, sigh.  This issue won't be present when the compilers we care about support C++11 static_assert().

So then, just the enclosing parentheses should be sufficient here.  And get rid of the comment; there's nothing we can do, doesn't seem worth calling out.

@@ +65,5 @@
> +      //XXX: the reason for the !! and the paranthesis is not known.
> +      MOZ_STATIC_ASSERT(!!(IsBaseOf<SupportsWeakPtr<T>, T>::value), "T must derive from SupportsWeakPtr<T>");
> +      if (weakRef) {
> +          weakRef->detach();
> +      }

mfbt doesn't parenthesize single-line ifs, and this should be two-space indented.

@@ +72,5 @@
> +  private:
> +
> +    friend class WeakPtr<T>;
> +
> +    // This can live beyond the lifetime of the class derived from SupportsWeakPtr

Period after a complete sentence, please.

@@ +73,5 @@
> +
> +    friend class WeakPtr<T>;
> +
> +    // This can live beyond the lifetime of the class derived from SupportsWeakPtr
> +    class WeakReference : public RefCounted<WeakReference> {

class WeakReference : ...
{

@@ +79,5 @@
> +        WeakReference(T* ptr) : ptr(ptr) {}
> +        T* get() {
> +          return ptr;
> +        }
> +      private:

Blank line between access-modifier sections, please, for readability.

@@ +102,5 @@
> +    operator T*() const {
> +      return ref->get();
> +    }
> +    T& operator*() const {
> +      return *(ref->get());

Remove the parentheses here, please.

@@ +113,5 @@
> +  private:
> +    friend class SupportsWeakPtr<T>;
> +
> +    //XXX: should this be taking a RefPtr?
> +    WeakPtr(RefPtr<typename SupportsWeakPtr<T>::WeakReference>  o) : ref(o) {}

Might be worth a |typedef typename SupportsWeakPtr<T>::WeakReference WeakReference;| since you repeat this twice.

::: mfbt/tests/TestWeakPtr.cpp
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Let's directly copy the example from the header into the test here and use that, as the next-best thing to having comments be parsed out of headers and tested directly.

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "mozilla/WeakPtr.h"
> +
> +using namespace mozilla;

I'm increasingly leery of whole-namespace using statements like this for cluttering reasons, and for making it harder to see dependencies.  Could you |using mozilla::WeakPtr| instead?

@@ +6,5 @@
> +
> +using namespace mozilla;
> +
> +struct A : public SupportsWeakPtr<A> {
> +    int a;

If you want to keep the set of tests that are already here as well, for extra assurance, go for it.  But please rename this so the class and field don't use the same letter.  :-)
Attachment #663108 - Flags: review?(jwalden+bmo) → review-
Oh, a mini-nit, but this should have a one-line comment describing what the file implements that's suitable for display in MXR, like all the other mfbt files have:

http://mxr.mozilla.org/mozilla-central/source/mfbt/

Something like this would probably do it:

/* Weak pointer functionality, implemented as a mixin for use with any class. */
Attachment #663108 - Attachment is obsolete: true
Attachment #667779 - Flags: review?(jwalden+bmo)
Blocks: 792199
Comment on attachment 667779 [details] [diff] [review]
Add a WeakPtr implementation to use instead of nsISupportsWeakReference v2

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

::: mfbt/WeakPtr.h
@@ +17,5 @@
> + * dereference, and an additional heap allocated pointer sized object shared
> + * between all of the WeakPtrs.
> + *
> + * Example of usage:
> + *   // To have a class C support weak pointers, inherit from SupportsWeakPtr<C>.

Blank line before start of example.

@@ +72,5 @@
> +  public:
> +    WeakPtr<T> asWeakPtr() {
> +      if (!weakRef) {
> +        weakRef = new WeakReference(static_cast<T*>(this));
> +      }

No braces here, if you're not going to keep the comment.

@@ +80,5 @@
> +  protected:
> +    ~SupportsWeakPtr() {
> +      MOZ_STATIC_ASSERT((IsBaseOf<SupportsWeakPtr<T>, T>::value), "T must derive from SupportsWeakPtr<T>");
> +      if (weakRef)
> +          weakRef->detach();

This should be two-space-indented, not four.

@@ +85,5 @@
> +    }
> +
> +  private:
> +
> +    friend class WeakPtr<T>;

No blank line before this.

@@ +130,5 @@
> +  private:
> +    friend class SupportsWeakPtr<T>;
> +
> +    //XXX: should this be taking a RefPtr?
> +    WeakPtr(RefPtr<typename SupportsWeakPtr<T>::WeakReference>  o) : ref(o) {}

Not sure I understand the XXX here, on second glance.  And is there a reason for this not to be passed by const reference?

@@ +135,5 @@
> +
> +    RefPtr<typename SupportsWeakPtr<T>::WeakReference> ref;
> +};
> +
> +}

Add a // namespace mozilla here.

::: mfbt/tests/TestWeakPtr.cpp
@@ +4,5 @@
> +
> +#include "mozilla/WeakPtr.h"
> +
> +using mozilla::WeakPtr;
> +using mozilla::SupportsWeakPtr;

Alphabetize these, please?

@@ +6,5 @@
> +
> +using mozilla::WeakPtr;
> +using mozilla::SupportsWeakPtr;
> +
> +void example()

mfbt style would have void on a separate line (and static, too).

@@ +14,5 @@
> +  {
> +    public:
> +      int num;
> +      void act() {}
> +  };

I don't think this works across compilers, to put this class definition here.  C++98 doesn't allow instantiating templates dependent on locally-defined types (C++11 rectifies this somewhat), so in a conformant C++98 compiler, I don't think this compiles.  I'm sure we have at least one compiler that does this, in at least some situations, so move the class definition to top level.

@@ +38,5 @@
> +  MOZ_ASSERT(!weak, "Deleting |ptr| clears weak pointers to it.");
> +  MOZ_ASSERT(!other, "Deleting |ptr| clears all weak pointers to it.");
> +
> +
> +}

Get rid of these blank lines.

@@ +40,5 @@
> +
> +
> +}
> +
> +struct A : public SupportsWeakPtr<A> {

{ on new line.

@@ +51,5 @@
> +
> +  A* a = new A;
> +
> +  // a2 is unused to test the case when we haven't initialized
> +  // the internal WeakReference pointer

Period at the end of this.

@@ +68,5 @@
> +  MOZ_ASSERT(!ptr);
> +
> +  delete a2;
> +
> +  return 0;

It looks like you forgot to add a call to example() somewhere here.
Attachment #667779 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/a2630fb2dbfa
Assignee: nobody → jmuizelaar
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.