882562 - Add unit test for RefPtr.h

Status: RESOLVED DUPLICATE of bug 1045920

(Core :: MFBT, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Add TestRefPtr

Includes coverage for / depends on the GenericRefCounted stuff from bug 882561.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 761885 [details] [diff] [review]
Add TestRefPtr

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

Could you remove the #if 0'd code from RefPtr.h when you do this?  At a skim, it looks like what's there isn't really going to cover more bases than are covered here.

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

Please |using mozilla::RefCounted;| and similar for the individual things you use.

@@ +11,5 @@
> + * have some object types to test with various
> + * reference-counting base classes.
> + */
> +template<typename RefCountedBase>
> +class Foo : public RefCountedBase

At least name it DummyRefCounted or something, please.  :-)

@@ +29,5 @@
> +    {
> +      sDestructed++;
> +    }
> +
> +    static void CheckConstructedDestructed(int c, int d)

camelCaps for checkConstructedDestructed, resetConstructedDestructed

@@ +31,5 @@
> +    }
> +
> +    static void CheckConstructedDestructed(int c, int d)
> +    {
> +      MOZ_ASSERT(sConstructed == c && sDestructed == d);

Split this into two asserts, for more clarity about which is hit, if it's ever hit.

@@ +34,5 @@
> +    {
> +      MOZ_ASSERT(sConstructed == c && sDestructed == d);
> +    }
> +
> +    static void ResetConstructedDestructed()

camelCaps

@@ +60,5 @@
> + * you want to test a non-templated RefCountedBase such as
> + * GenericRefCountedBase. But what if RefCountedBase is
> + * a CRTP-style templated base, such as RefCounted<T>?
> + *
> + * Watch this, kids:

These comments are classic.

@@ +63,5 @@
> + *
> + * Watch this, kids:
> + */
> +template <template <typename T> class RefcountedBase>
> +class CRTPFoo

CRTPDummyRefCounted

@@ +75,5 @@
> +    {}
> +};
> +
> +template<typename ObjectType>
> +void TestBasics()

template<typename ObjectType>
static void
TestBasics()
{

Same elsewhere too.

@@ +110,5 @@
> +void TestAssign()
> +{
> +  ObjectType::CheckConstructedDestructed(0, 0);
> +
> +  const int num_objects = 4;

Use size_t since we're counting stuff.

@@ +122,5 @@
> +  ObjectType::CheckConstructedDestructed(num_objects, 0);
> +
> +  for (int shift = 1; shift < num_objects; shift++) {
> +    // shift p[i] pointers. Should destroy one object.
> +    for (int i = 0; i < num_objects - shift; i++) {

size_t for shift/i.

Comment 2

[Simon Giesecke [:sg] [he/him]]

Some tests for RefPtr landed with Bug 1045920. I guess this can be closed now? If there are additions here that should be tested, please reopen and adapt the description of this bug.