[gjs: 1/4] GjsMaybeOwned: Don't use union for root and heap

[Philip Chimento <pchimento src gnome org>]

commit f2879d52a51b03b80fe29af48d9a0e488f2c21d8
Author: Marco Trevisan (Treviño) <mail 3v1n0 net>
Date:   Mon Jun 17 22:35:46 2019 +0200

    GjsMaybeOwned: Don't use union for root and heap
    
    Using an union and a boolean used to check which member uses the same memory
    of two instances, so just use private members using the pointer to check
    the rooted state.

 gjs/jsapi-util-root.h | 115 ++++++++++++++++++++++++--------------------------
 1 file changed, 54 insertions(+), 61 deletions(-)
---
diff --git a/gjs/jsapi-util-root.h b/gjs/jsapi-util-root.h
index 1358b382..b8d298b9 100644
--- a/gjs/jsapi-util-root.h
+++ b/gjs/jsapi-util-root.h
@@ -1,6 +1,7 @@
 /* -*- mode: C++; c-basic-offset: 4; indent-tabs-mode: nil; -*- */
 /*
  * Copyright (c) 2017 Endless Mobile, Inc.
+ * Copyright (c) 2019 Canonical, Ltd.
  *
  * Permission is hereby granted, free of charge, to any person obtaining a copy
  * of this software and associated documentation files (the "Software"), to
@@ -27,6 +28,7 @@
 #include <stdint.h>  // for uintptr_t
 
 #include <cstddef>  // for nullptr_t
+#include <memory>
 #include <new>
 #include <type_traits>  // for enable_if_t, is_pointer
 
@@ -114,25 +116,19 @@ struct GjsHeapOperation<JSFunction*> {
  * member of a heap-allocated struct. */
 template<typename T>
 class GjsMaybeOwned {
-public:
+ public:
     typedef void (*DestroyNotify)(JS::Handle<T> thing, void *data);
 
-private:
-    bool m_rooted;  /* wrapper is in rooted mode */
+ private:
     bool m_has_weakref;  /* we have a weak reference to the GjsContext */
 
     JSContext *m_cx;
 
-    /* m_rooted controls which of these members we can access. When switching
+    /* m_root value controls which of these members we can access. When switching
      * from one to the other, be careful to call the constructor and destructor
      * of JS::Heap, since they use post barriers. */
-    union RootUnion {
-        JS::Heap<T> heap;
-        JS::PersistentRooted<T>* root;
-
-        RootUnion() : heap() {}
-        ~RootUnion() {}
-    } m_thing;
+    JS::Heap<T> m_heap;
+    std::unique_ptr<JS::PersistentRooted<T>> m_root;
 
     DestroyNotify m_notify;
     void *m_data;
@@ -151,11 +147,10 @@ private:
 
     void teardown_rooting() {
         debug("teardown_rooting()");
-        g_assert(m_rooted);
+        g_assert(m_root);
 
-        delete m_thing.root;
-        new (&m_thing.heap) JS::Heap<T>();
-        m_rooted = false;
+        m_root.reset();
+        new (&m_heap) JS::Heap<T>();
 
         if (!m_has_weakref)
             return;
@@ -173,7 +168,7 @@ private:
     invalidate(void)
     {
         debug("invalidate()");
-        g_assert(m_rooted);
+        g_assert(m_root);
 
         /* The weak ref is already gone because the context is dead, so no need
          * to remove it. */
@@ -187,24 +182,23 @@ private:
             reset();
     }
 
-public:
-    GjsMaybeOwned(void) :
-        m_rooted(false),
-        m_has_weakref(false),
-        m_cx(nullptr),
-        m_notify(nullptr),
-        m_data(nullptr)
-    {
+ public:
+    GjsMaybeOwned()
+        : m_has_weakref(false),
+          m_cx(nullptr),
+          m_root(nullptr),
+          m_notify(nullptr),
+          m_data(nullptr) {
         debug("created");
     }
 
     ~GjsMaybeOwned() {
         debug("destroyed");
-        if (m_rooted)
+        if (m_root)
             teardown_rooting();
 
         /* Call in either case; teardown_rooting() constructs a new Heap */
-        m_thing.heap.~Heap();
+        m_heap.~Heap();
     }
 
     /* To access the GC thing, call get(). In many cases you can just use the
@@ -212,7 +206,7 @@ public:
      * cast operator. But if you want to call methods on the GC thing, for
      * example if it's a JS::Value, you have to use get(). */
     GJS_USE const T get() const {
-        return m_rooted ? m_thing.root->get() : m_thing.heap.get();
+        return m_root ? m_root->get() : m_heap.get();
     }
     operator const T() const { return get(); }
 
@@ -220,15 +214,15 @@ public:
     template <typename U = T>
     GJS_USE const T
     debug_addr(std::enable_if_t<std::is_pointer<U>::value>* = nullptr) const {
-        return m_rooted ? m_thing.root->get() : m_thing.heap.unbarrieredGet();
+        return m_root ? m_root->get() : m_heap.unbarrieredGet();
     }
 
     bool
     operator==(const T& other) const
     {
-        if (m_rooted)
-            return m_thing.root->get() == other;
-        return m_thing.heap == other;
+        if (m_root)
+            return m_root->get() == other;
+        return m_heap == other;
     }
     inline bool operator!=(const T& other) const { return !(*this == other); }
 
@@ -237,9 +231,9 @@ public:
     bool
     operator==(std::nullptr_t) const
     {
-        if (m_rooted)
-            return m_thing.root->get() == nullptr;
-        return m_thing.heap.unbarrieredGet() == nullptr;
+        if (m_root)
+            return m_root->get() == nullptr;
+        return m_heap.unbarrieredGet() == nullptr;
     }
     inline bool operator!=(std::nullptr_t) const { return !(*this == nullptr); }
 
@@ -250,8 +244,8 @@ public:
      * wrapper with stack rooting. However, you must not do this if the
      * JSContext can be destroyed while the Handle is live. */
     GJS_USE JS::Handle<T> handle() {
-        g_assert(m_rooted);
-        return *m_thing.root;
+        g_assert(m_root);
+        return *m_root;
     }
 
     /* Roots the GC thing. You must not use this if you're already using the
@@ -263,14 +257,13 @@ public:
          void         *data   = nullptr)
     {
         debug("root()");
-        g_assert(!m_rooted);
-        g_assert(m_thing.heap.get() == JS::GCPolicy<T>::initial());
-        m_rooted = true;
+        g_assert(!m_root);
+        g_assert(m_heap.get() == JS::GCPolicy<T>::initial());
         m_cx = cx;
         m_notify = notify;
         m_data = data;
-        m_thing.heap.~Heap();
-        m_thing.root = new JS::PersistentRooted<T>(m_cx, thing);
+        m_heap.~Heap();
+        m_root = std::make_unique<JS::PersistentRooted<T>>(m_cx, thing);
 
         if (notify) {
             GjsContextPrivate* gjs = GjsContextPrivate::from_cx(m_cx);
@@ -286,8 +279,8 @@ public:
     void
     operator=(const T& thing)
     {
-        g_assert(!m_rooted);
-        m_thing.heap = thing;
+        g_assert(!m_root);
+        m_heap = thing;
     }
 
     /* Marks an object as reachable for one GC with ExposeObjectToActiveJS().
@@ -295,14 +288,14 @@ public:
      * in the rooted case. */
     void prevent_collection() {
         debug("prevent_collection()");
-        g_assert(!m_rooted);
-        GjsHeapOperation<T>::expose_to_js(m_thing.heap);
+        g_assert(!m_root);
+        GjsHeapOperation<T>::expose_to_js(m_heap);
     }
 
     void reset() {
         debug("reset()");
-        if (!m_rooted) {
-            m_thing.heap = JS::GCPolicy<T>::initial();
+        if (!m_root) {
+            m_heap = JS::GCPolicy<T>::initial();
             return;
         }
 
@@ -318,30 +311,30 @@ public:
                      void         *data   = nullptr)
     {
         debug("switch to rooted");
-        g_assert(!m_rooted);
+        g_assert(!m_root);
 
         /* Prevent the thing from being garbage collected while it is in neither
-         * m_thing.heap nor m_thing.root */
+         * m_heap nor m_root */
         JSAutoRequest ar(cx);
-        JS::Rooted<T> thing(cx, m_thing.heap);
+        JS::Rooted<T> thing(cx, m_heap);
 
         reset();
         root(cx, thing, notify, data);
-        g_assert(m_rooted);
+        g_assert(m_root);
     }
 
     void switch_to_unrooted() {
         debug("switch to unrooted");
-        g_assert(m_rooted);
+        g_assert(m_root);
 
         /* Prevent the thing from being garbage collected while it is in neither
-         * m_thing.heap nor m_thing.root */
+         * m_heap nor m_root */
         JSAutoRequest ar(m_cx);
-        JS::Rooted<T> thing(m_cx, *m_thing.root);
+        JS::Rooted<T> thing(m_cx, *m_root);
 
         reset();
-        m_thing.heap = thing;
-        g_assert(!m_rooted);
+        m_heap = thing;
+        g_assert(!m_root);
     }
 
     /* Tracing makes no sense in the rooted case, because JS::PersistentRooted
@@ -351,8 +344,8 @@ public:
           const char *name)
     {
         debug("trace()");
-        g_assert(!m_rooted);
-        JS::TraceEdge<T>(tracer, &m_thing.heap, name);
+        g_assert(!m_root);
+        JS::TraceEdge<T>(tracer, &m_heap, name);
     }
 
     /* If not tracing, then you must call this method during GC in order to
@@ -360,11 +353,11 @@ public:
      * finalized. If the object was finalized, returns true. */
     GJS_USE bool update_after_gc() {
         debug("update_after_gc()");
-        g_assert(!m_rooted);
-        return GjsHeapOperation<T>::update_after_gc(&m_thing.heap);
+        g_assert(!m_root);
+        return GjsHeapOperation<T>::update_after_gc(&m_heap);
     }
 
-    GJS_USE bool rooted() const { return m_rooted; }
+    GJS_USE bool rooted() const { return m_root != nullptr; }
 };
 
 #endif  // GJS_JSAPI_UTIL_ROOT_H_