Issue 4125 - [emscripten] Convert filter classes to C++

compiled/intrusive_ptr.h

 // Parts of this code have been copied from boost/smart_ptr/intrusive_ptr.hpp.
 //
 //  Copyright (c) 2001, 2002 Peter Dimov
 //
 // Distributed under the Boost Software License, Version 1.0. (See
 // accompanying file LICENSE_1_0.txt or copy at
 // http://www.boost.org/LICENSE_1_0.txt)
 
 #pragma once
 
 #include <algorithm>
 #include <type_traits>
 
+#include "debug.h"
+
 class ref_counted
 {
 public:
   void AddRef()
   {
     mRefCount++;
   }
 
   void ReleaseRef()
   {

[sergei, 2016/06/16 21:17:29]

It would be good to have assert that mRefCount is not zero.

[Wladimir Palant, 2016/12/06 10:48:29]

Done.

+    assert(mRefCount > 0, u"Unexpected zero or negative reference count"_str);
     if (--mRefCount == 0)
       delete this;
   }
 
 protected:
   ref_counted()
-      : mRefCount(0)

[sergei, 2016/06/16 21:17:24]

To tell the truth I'm not sure that the initial value should be zero.

[Wladimir Palant, 2016/12/06 10:48:24]

Yes, I changed that already along with the general ref counting changes.

-  {
-  }
-
-  // We need this virtual destructor, otherwise pointers to ref_counted and

[sergei, 2016/06/16 21:17:18]

This comment is incorrect, pointers are equal, so it would be better to remove
it.

JIC, the destructor should be virtual because otherwise the destructor of
derived object will not be called from ref_counted::ReleaseRef.

[Wladimir Palant, 2016/12/06 10:48:28]

Pointers are equal if you compare them in C++. However, if you convert them to
an integer, a Filter* and a ref_counted* pointer to the same object don't have
the same value. This is actually problematic for our JavaScript bindings that
don't know how to convert pointers. I tried improving the comment in order to
make the issue more obvious.
Even though the constructor of the derived object is virtual?

[sergei, 2017/01/10 15:57:22]

Pointers in c++ are already numeric values and are not needed to be converted.
I think I see the issue, however I still think that we don't need that comment
because it is confusing and addresses only one case of a well known behavior of
C++ compiler.
Let's say we have class A{int a1;int a2;}; class B{int b1;int b2;}; and class C
: public B, public A{}; In that case layout of object C in memory is
C B int b1;
    int b2;
  A int a1;
    int a2;
in this case &c - static_cast<A*>(&c) = 8 (here and below size of int is 4
bytes), however &c - static_cast<B*>(&c) = 0.
Now let's add a virtual destructor for A and keep the same inheritance order.
The layout of C is
C A vtable with ~A (for 64 platform its size is 8 bytes, here and below)
    int a1;
    int a2;
  B int b1;
    int b2;
now
&c - static_cast<A*>(&c) = 0 (what the comment says), and &c -
static_cast<B*>(&c) = 16.
Now let's add virtual destructor for B. The layout of C is
C B vtable with ~B
    int b1;
    int b2;
  A vtable with ~A
    int a1;
    int a2;
&c - static_cast<A*>(&c) = 16 (comment is again wrong), and &c -
static_cast<B*>(&c) = 0.
There is no such concept as virtual constructor in C++.

[Wladimir Palant, 2017/03/13 17:41:46]

Ok, I removed the comment here and instead added one in bindings.ipp - that's
where we have the issue, and we should keep it in mind.

[Wladimir Palant, 2017/03/14 10:23:12]

Actually, it is better to remove the footgun instead of commenting on it. I
realized that this is possible, with Patch Set 28 we will handle offsets
correctly.

-  // pointers to derived classes won't have the same value (e.g. converting from
-  // Filter* to ref_counted* decreases pointer value by 4).
+      : mRefCount(1)
+  {
+  }
+
   virtual ~ref_counted()
   {

[sergei, 2016/06/16 21:17:20]

it would be good to have assert that mRefCount is zero.

[Wladimir Palant, 2016/12/06 10:48:22]

Done.

+    assert(mRefCount == 0, u"Destroying a ref-counted object with a non-zero reference count"_str);
   }
 
 private:
   int mRefCount;
 };
 
 template<typename T,
     class = typename std::enable_if<std::is_base_of<ref_counted,T>::value>::type>
 class intrusive_ptr
 {
 public:
-  intrusive_ptr()
+  explicit intrusive_ptr()
       : mPointer(nullptr)
   {
   }
 
-  intrusive_ptr(T* pointer)
+  explicit intrusive_ptr(T* pointer)
       : mPointer(pointer)
+  {
+    // Raw pointers always had their reference count increased by whatever gave
+    // us the pointer so we don't need to do it here.
+  }
+
+  intrusive_ptr(const intrusive_ptr& other)
+      : mPointer(other.mPointer)
   {
     if (mPointer)
       mPointer->AddRef();
   }
 
-  intrusive_ptr(const intrusive_ptr& other)
-      : mPointer(other.mPointer)
-  {
-    if (mPointer)
-      mPointer->AddRef();
-  }
-
   intrusive_ptr(intrusive_ptr&& other)
       : mPointer(other.mPointer)
   {
     other.mPointer = nullptr;
   }
 
   ~intrusive_ptr()
   {
     if (mPointer)
       mPointer->ReleaseRef();
@@ skipping 50 matching lines @@
   const T* operator->() const
   {
     return mPointer;
   }
 
   T* operator->()
   {
     return mPointer;
   }
 
-  operator bool() const

[sergei, 2016/06/16 21:17:27]

It would be also better to make it explicit operator bool.

[Wladimir Palant, 2016/12/06 10:48:26]

Done.

+  explicit operator bool() const
   {
     return mPointer != nullptr;
   }
 
-  operator T*() const

[sergei, 2016/06/16 21:17:22]

I personally don't like this operator because of its danger, it would be safer
to call .get() each time we need raw pointer.

[Wladimir Palant, 2016/12/06 10:48:16]

Done.

-  {
-    return mPointer;
-  }
-
   bool operator!() const
   {
     return mPointer == nullptr;
+  }
+
+  T* release()
+  {
+    T* result = mPointer;
+    mPointer = nullptr;
+    return result;
   }
 
   void swap(intrusive_ptr& other)
   {
     std::swap(mPointer, other.mPointer);
   }
 
 private:
   T* mPointer;
 };
 
 template<typename T, typename U>
 inline bool operator==(const intrusive_ptr<T>& a, const intrusive_ptr<U>& b)

[sergei, 2016/06/16 21:17:21]

JIC, these operators don't always work, e.g. with multiple inheritance when base
classes have virtual table. But I think we can leave them as is. May be casting
pointers to "const ref_counted*" before comparison could solve the issue but it
seems these methods are not used anyway now.

[Wladimir Palant, 2016/12/06 10:48:21]

Won't C++ cast these pointers automatically in order to compare them?

[sergei, 2017/01/10 15:57:20]

No.

 {
   return a.get() == b.get();
 }
 
 template<typename T, typename U>
 inline bool operator!=(const intrusive_ptr<T>& a, const intrusive_ptr<U>& b)
 {
   return a.get() != b.get();
 }
 
@@ skipping 13 matching lines @@
 inline bool operator==(const T* a, const intrusive_ptr<U>& b)
 {
   return a == b.get();
 }
 
 template<typename T, typename U>
 inline bool operator!=(const T* a, intrusive_ptr<U> const& b)
 {
   return a != b.get();
 }