libshikoken: Refactor the Visitor class hierarchy

Implementing the deleteInMainThread feature requires being able to
collect a list of destructors and potentially storing it. This
requires splitting out the actual deallocation/destructor calls from
the DtorCallerVisitor and DeallocVisitor classes.

Since this is the only use of the virtual HierarchyVisitor::done()
method (and it does not really belong to the visitor pattern), remove
it.

Change the void visit() method into a bool from which true can be
returned to terminate. The finish()/wasFinished() methods can then
also be removed from HierarchyVisitor and the code simplified
accordingly.

Replace the DtorCallerVisitor and DeallocVisitor classes
by DtorAccumulatorVisitor that collects a list of DestructorEntry
structs containing destructor function and C++ instance.

Polish the code a bit, use member initialization, add override, move
implementations to source and some spacing for clarity.

Task-number: PYSIDE-810
Change-Id: I5e3ef6df753679ec111a5f0d1b75305bd5cf1c0c
Reviewed-by: Christian Tismer <tismer@stackless.com>

2 files changed, 95 insertions, 93 deletions

diff --git a/sources/shiboken2/libshiboken/basewrapper.cpp b/sources/shiboken2/libshiboken/basewrapper.cpp
index 81830952a..2b27f2881 100644
--- a/sources/shiboken2/libshiboken/basewrapper.cpp
+++ b/sources/shiboken2/libshiboken/basewrapper.cpp
@@ -60,6 +60,15 @@ namespace {
     void _destroyParentInfo(SbkObject* obj, bool keepReference);
 }
 
+static void callDestructor(const Shiboken::DtorAccumulatorVisitor::DestructorEntries &dts)
+{
+    for (const auto &e : dts) {
+        Shiboken::ThreadStateSaver threadSaver;
+        threadSaver.save();
+        e.destructor(e.cppInstance);
+    }
+}
+
 extern "C"
 {
 
@@ -212,8 +221,10 @@ static void SbkDeallocWrapperCommon(PyObject* pyObj, bool canDelete)
     if (canDelete && sbkObj->d->hasOwnership && sbkObj->d->validCppObject) {
         SbkObjectTypePrivate *sotp = PepType_SOTP(pyType);
         if (sotp->is_multicpp) {
-            Shiboken::DeallocVisitor visitor(sbkObj);
+            Shiboken::DtorAccumulatorVisitor visitor(sbkObj);
             Shiboken::walkThroughClassHierarchy(Py_TYPE(pyObj), &visitor);
+            Shiboken::Object::deallocData(sbkObj, true);
+            callDestructor(visitor.entries());
         } else {
             void* cptr = sbkObj->d->cptr[0];
             Shiboken::Object::deallocData(sbkObj, true);
@@ -447,34 +458,22 @@ void _destroyParentInfo(SbkObject* obj, bool keepReference)
 
 namespace Shiboken
 {
-static void _walkThroughClassHierarchy(PyTypeObject* currentType, HierarchyVisitor* visitor)
+bool walkThroughClassHierarchy(PyTypeObject *currentType, HierarchyVisitor *visitor)
 {
     PyObject* bases = currentType->tp_bases;
     Py_ssize_t numBases = PyTuple_GET_SIZE(bases);
-    for (int i = 0; i < numBases; ++i) {
+    bool result = false;
+    for (int i = 0; !result && i < numBases; ++i) {
         PyTypeObject* type = reinterpret_cast<PyTypeObject*>(PyTuple_GET_ITEM(bases, i));
-
-        if (!PyType_IsSubtype(type, reinterpret_cast<PyTypeObject*>(SbkObject_TypeF()))) {
-            continue;
-        } else {
+        if (PyType_IsSubtype(type, reinterpret_cast<PyTypeObject*>(SbkObject_TypeF()))) {
             SbkObjectType* sbkType = reinterpret_cast<SbkObjectType*>(type);
-            if (PepType_SOTP(sbkType)->is_user_type)
-                _walkThroughClassHierarchy(type, visitor);
-            else
-                visitor->visit(sbkType);
+            result = PepType_SOTP(sbkType)->is_user_type
+                     ? walkThroughClassHierarchy(type, visitor) : visitor->visit(sbkType);
         }
-        if (visitor->wasFinished())
-            break;
     }
+    return result;
 }
 
-void walkThroughClassHierarchy(PyTypeObject* currentType, HierarchyVisitor* visitor)
-{
-    _walkThroughClassHierarchy(currentType, visitor);
-    visitor->done();
-}
-
-
 bool importModule(const char* moduleName, PyTypeObject*** cppApiPtr)
 {
     PyObject* sysModules = PyImport_GetModuleDict();
@@ -506,24 +505,32 @@ bool importModule(const char* moduleName, PyTypeObject*** cppApiPtr)
 
 // Wrapper metatype and base type ----------------------------------------------------------
 
-void DtorCallerVisitor::visit(SbkObjectType* node)
+HierarchyVisitor::HierarchyVisitor() = default;
+HierarchyVisitor::~HierarchyVisitor() = default;
+
+bool BaseCountVisitor::visit(SbkObjectType *)
 {
-    m_ptrs.push_back(std::make_pair(m_pyObj->d->cptr[m_ptrs.size()], node));
+    m_count++;
+    return false;
 }
 
-void DtorCallerVisitor::done()
+bool BaseAccumulatorVisitor::visit(SbkObjectType *node)
 {
-    for (auto it = m_ptrs.begin(), end = m_ptrs.end(); it != end; ++it) {
-        Shiboken::ThreadStateSaver threadSaver;
-        threadSaver.save();
-        PepType_SOTP(it->second)->cpp_dtor(it->first);
-    }
+    m_bases.push_back(node);
+    return false;
 }
 
-void DeallocVisitor::done()
+bool GetIndexVisitor::visit(SbkObjectType *node)
 {
-    Shiboken::Object::deallocData(m_pyObj, true);
-    DtorCallerVisitor::done();
+    m_index++;
+    return PyType_IsSubtype(reinterpret_cast<PyTypeObject*>(node), m_desiredType);
+}
+
+bool DtorAccumulatorVisitor::visit(SbkObjectType *node)
+{
+    m_entries.push_back(DestructorEntry{PepType_SOTP(node)->cpp_dtor,
+                                        m_pyObject->d->cptr[m_entries.size()]});
+    return false;
 }
 
 namespace Conversions { void init(); }
@@ -597,20 +604,16 @@ void setErrorAboutWrongArguments(PyObject* args, const char* funcName, const cha
 
 class FindBaseTypeVisitor : public HierarchyVisitor
 {
-    public:
-        FindBaseTypeVisitor(PyTypeObject* typeToFind) : m_found(false), m_typeToFind(typeToFind) {}
-        virtual void visit(SbkObjectType* node)
-        {
-            if (reinterpret_cast<PyTypeObject*>(node) == m_typeToFind) {
-                m_found = true;
-                finish();
-            }
-        }
-        bool found() const { return m_found; }
+public:
+    explicit FindBaseTypeVisitor(PyTypeObject *typeToFind) : m_typeToFind(typeToFind) {}
+
+    bool visit(SbkObjectType *node) override
+    {
+        return reinterpret_cast<PyTypeObject*>(node) == m_typeToFind;
+    }
 
-    private:
-        bool m_found;
-        PyTypeObject* m_typeToFind;
+private:
+    PyTypeObject *m_typeToFind;
 };
 
 std::vector<SbkObject *> splitPyObject(PyObject* pyObj)
@@ -654,8 +657,7 @@ bool isUserType(PyTypeObject* type)
 bool canCallConstructor(PyTypeObject* myType, PyTypeObject* ctorType)
 {
     FindBaseTypeVisitor visitor(ctorType);
-    walkThroughClassHierarchy(myType, &visitor);
-    if (!visitor.found()) {
+    if (!walkThroughClassHierarchy(myType, &visitor)) {
         PyErr_Format(PyExc_TypeError, "%s isn't a direct base class of %s", ctorType->tp_name, myType->tp_name);
         return false;
     }
@@ -871,8 +873,9 @@ void callCppDestructors(SbkObject* pyObj)
     PyTypeObject *type = Py_TYPE(pyObj);
     SbkObjectTypePrivate * sotp = PepType_SOTP(type);
     if (sotp->is_multicpp) {
-        Shiboken::DtorCallerVisitor visitor(pyObj);
+        Shiboken::DtorAccumulatorVisitor visitor(pyObj);
         Shiboken::walkThroughClassHierarchy(type, &visitor);
+        callDestructor(visitor.entries());
     } else {
         Shiboken::ThreadStateSaver threadSaver;
         threadSaver.save();
diff --git a/sources/shiboken2/libshiboken/basewrapper_p.h b/sources/shiboken2/libshiboken/basewrapper_p.h
index a61d5c947..d99ca19ea 100644
--- a/sources/shiboken2/libshiboken/basewrapper_p.h
+++ b/sources/shiboken2/libshiboken/basewrapper_p.h
@@ -150,6 +150,17 @@ struct SbkObjectTypePrivate
 
 namespace Shiboken
 {
+
+/**
+ * \internal
+ * Data required to invoke a C++ destructor
+ */
+struct DestructorEntry
+{
+    ObjectDestructor destructor;
+    void *cppInstance;
+};
+
 /**
  * Utility function used to transform a PyObject that implements sequence protocol into a std::list.
  **/
@@ -161,29 +172,26 @@ std::vector<SbkObject *> splitPyObject(PyObject* pyObj);
 class HierarchyVisitor
 {
 public:
-    HierarchyVisitor() : m_wasFinished(false) {}
-    virtual ~HierarchyVisitor() {}
-    virtual void visit(SbkObjectType* node) = 0;
-    virtual void done() {}
-    void finish() { m_wasFinished = true; };
-    bool wasFinished() const { return m_wasFinished; }
-private:
-    bool m_wasFinished;
+    HierarchyVisitor(const HierarchyVisitor &) = delete;
+    HierarchyVisitor(HierarchyVisitor &&) = delete;
+    HierarchyVisitor &operator=(const HierarchyVisitor &) = delete;
+    HierarchyVisitor &operator=(HierarchyVisitor &&) = delete;
+
+    HierarchyVisitor();
+    virtual ~HierarchyVisitor();
+
+    virtual bool visit(SbkObjectType *node) = 0; // return true to terminate
 };
 
 class BaseCountVisitor : public HierarchyVisitor
 {
 public:
-    BaseCountVisitor() : m_count(0) {}
-
-    void visit(SbkObjectType*)
-    {
-        m_count++;
-    }
+    bool visit(SbkObjectType *) override;
 
     int count() const { return m_count; }
+
 private:
-    int m_count;
+    int m_count = 0;
 };
 
 class BaseAccumulatorVisitor : public HierarchyVisitor
@@ -191,14 +199,10 @@ class BaseAccumulatorVisitor : public HierarchyVisitor
 public:
     typedef std::vector<SbkObjectType *> Result;
 
-    BaseAccumulatorVisitor() {}
-
-    void visit(SbkObjectType* node)
-    {
-        m_bases.push_back(node);
-    }
+    bool visit(SbkObjectType *node) override;
 
     Result bases() const { return m_bases; }
+
 private:
     Result m_bases;
 };
@@ -206,38 +210,33 @@ private:
 class GetIndexVisitor : public HierarchyVisitor
 {
 public:
-    GetIndexVisitor(PyTypeObject* desiredType) : m_index(-1), m_desiredType(desiredType) {}
-    virtual void visit(SbkObjectType* node)
-    {
-        m_index++;
-        if (PyType_IsSubtype(reinterpret_cast<PyTypeObject*>(node), m_desiredType))
-            finish();
-    }
+    explicit GetIndexVisitor(PyTypeObject* desiredType) : m_desiredType(desiredType) {}
+
+    bool visit(SbkObjectType *node) override;
+
     int index() const { return m_index; }
 
 private:
-    int m_index;
-    PyTypeObject* m_desiredType;
+    int m_index = -1;
+    PyTypeObject *m_desiredType;
 };
 
-/// Call the destructor of each C++ object held by a Python object
-class DtorCallerVisitor : public HierarchyVisitor
+/// Collect destructors and C++ instances of each C++ object held by a Python
+/// object
+class DtorAccumulatorVisitor : public HierarchyVisitor
 {
 public:
-    DtorCallerVisitor(SbkObject* pyObj) : m_pyObj(pyObj) {}
-    void visit(SbkObjectType* node);
-    void done();
-protected:
-    std::vector<std::pair<void *, SbkObjectType *> > m_ptrs;
-    SbkObject* m_pyObj;
-};
+    explicit DtorAccumulatorVisitor(SbkObject *pyObj) : m_pyObject(pyObj) {}
 
-/// Dealloc of each C++ object held by a Python object, this implies a call to the C++ object destructor
-class DeallocVisitor : public DtorCallerVisitor
-{
-public:
-    DeallocVisitor(SbkObject* pyObj) : DtorCallerVisitor(pyObj) {}
-    void done();
+    bool visit(SbkObjectType *node) override;
+
+    using DestructorEntries = std::vector<DestructorEntry>;
+
+    const DestructorEntries &entries() const { return m_entries; }
+
+private:
+    DestructorEntries m_entries;
+    SbkObject *m_pyObject;
 };
 
 /// \internal Internal function used to walk on classes inheritance trees.
@@ -246,7 +245,7 @@ public:
 *   For each pure Shiboken type found, HiearchyVisitor::visit is called and the algorithm consider
 *   all children of this type as visited.
 */
-void walkThroughClassHierarchy(PyTypeObject* currentType, HierarchyVisitor* visitor);
+bool walkThroughClassHierarchy(PyTypeObject *currentType, HierarchyVisitor *visitor);
 
 inline int getTypeIndexOnHierarchy(PyTypeObject* baseType, PyTypeObject* desiredType)
 {