Merge branch 'standardization' into 'main'

Remove CParameter memory leak

See merge request eclipse/aidge/aidge_core!15

include/aidge/operator/GenericOperator.hpp

@@ -62,7 +62,12 @@ class GenericOperator_Op
      * @return template<class T> The parameter.
      */
     template <class T>
-    T getParameter(std::string const &key) const {
+    const T& getParameter(std::string const &key) const {
+        return mParams.Get<const T>(key);
+    }
+
+    template <class T>
+    T& getParameter(std::string const &key) {
         return mParams.Get<T>(key);
     }
 
@@ -75,8 +80,8 @@ class GenericOperator_Op
     /// internal buffer in a new location (previous value is still in memory at
     /// its previous location)
     template <class T>
-    void addParameter(std::string const &key, T const &value) {
-        mParams.Add<T>(key, value);
+    void addParameter(std::string const &key, T&& value) {
+        mParams.Add<T>(key, std::forward<T>(value));
     }
 
 

include/aidge/utils/Any.hpp

+
+/********************************************************************************
+ * Copyright (c) 2023 CEA-List
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Public License 2.0 which is available at
+ * http://www.eclipse.org/legal/epl-2.0.
+ *
+ * SPDX-License-Identifier: EPL-2.0
+ *
+ ********************************************************************************/
+
+#ifndef AIDGE_ANY_H_
+#define AIDGE_ANY_H_
+
+
+#include <typeinfo>    // typeid
+#include <type_traits> // std::enable_if_t, std::decay_t, std::is_same, std::is_copy_constructible, std::remove_cv, std::remove_reference
+#include <assert.h>
+#include <new>
+
+class _any {
+private:
+    /// @brief Operation to perform on the object.
+    enum _Op { _Op_access, _Op_get_type_info, _Op_clone, _Op_destroy };
+
+    union _Arg {
+        const std::type_info* _M_typeinfo;
+        _any* _M_any;
+    };
+
+    /// @brief Stored data without type information.
+    void* _M_data;
+
+    /// @brief Member function to perform type-related computations on stored data.
+    void (*_M_manager)(_Op, const _any*, _Arg*);
+
+public:
+    /// @brief Class to centralize functions and type information in a memory efficient way.
+    /// @tparam Tp Decayed stored type.
+    template <typename Tp>
+    struct Manager {
+        static void manage(_Op which, const _any* __any, _Arg* __arg) {
+            auto ptr = static_cast<const Tp*>(__any->_M_data);
+            switch (which)
+            {
+            case _Op_get_type_info:
+                __arg->_M_typeinfo = &typeid(Tp);
+                break;
+            case _Op_clone:
+                __arg->_M_any->_M_data = new Tp(*ptr);
+                __arg->_M_any->_M_manager = __any->_M_manager;
+                break;
+            case _Op_destroy:
+                delete ptr;
+                break;
+            }
+
+        }
+        static Tp* access(const _any* __any) {
+            return static_cast<Tp*>(__any->_M_data);
+        }
+
+        // template <typename Up>
+        // static void create(void* data, Up&& value) {
+        //     data = new Tp(std::forward<Up>(value));
+        // }
+    };
+
+private:
+    template<typename _Tp, typename _VTp = std::decay_t<_Tp>>
+    using _Decay_if_not_any = std::enable_if_t<!std::is_same<_VTp, _any>::value, _VTp>;
+
+public:
+    /// @brief Default constructor
+    _any() noexcept : _M_manager(nullptr) { }
+
+    /// @brief Copy constructor
+    /// @param __other
+    _any(const _any& __other)
+    {
+        if (!__other._M_manager)
+            _M_manager = nullptr;
+        else
+        {
+            _Arg __arg;
+            __arg._M_any = this;
+            __other._M_manager(_Op_clone, &__other, &__arg);
+        }
+    }
+
+    /// @brief Move constructor
+    /// @param __other
+    _any(_any&& __other)
+    {
+        if (!__other._M_manager)
+            _M_manager = nullptr;
+        else
+        {
+            _M_data = __other._M_data;
+            _M_manager = __other._M_manager;
+            const_cast<_any*>(&__other)->_M_manager = nullptr;
+        }
+    }
+
+    /// @brief By-value constructor.
+    /// @tparam T Data type.
+    /// @tparam VT Decayed data type.
+    /// @param value
+    template<typename T, typename VT = _Decay_if_not_any<T>, std::enable_if_t<std::is_copy_constructible<VT>::value, bool> = true>
+    explicit _any(T&& value)
+        : _M_manager(&Manager<VT>::manage),
+          _M_data(new VT{std::forward<T>(value)})
+    {}
+
+    ~_any()
+    {
+        if(_M_manager) {
+            _M_manager(_Op_destroy, this, nullptr);
+            _M_manager = nullptr;
+        }
+    }
+
+    /// @brief Access type id of the value currently stored
+    /// @return
+    const std::type_info& type() const
+    {
+        if (!_M_manager)
+            return typeid(void);
+        _Arg __arg;
+        _M_manager(_Op_get_type_info, this, &__arg);
+        return *__arg._M_typeinfo;
+    }
+};
+
+/// @brief Access value stored in the object converted in the template type if possible.
+/// @tparam _ValueType
+/// @param __any
+/// @return Stored value.
+template<typename _ValueType>
+inline _ValueType any_cast(const _any& __any)
+{
+    using _Up =  std::remove_cv_t<std::remove_reference_t<_ValueType>>;
+    assert((std::__or_<std::is_reference<_ValueType>, std::is_copy_constructible<_ValueType>>::value && "Template argument must be a reference or CopyConstructible type"));
+    assert((std::is_constructible<_ValueType, const _Up&>::value && "Template argument must be constructible from a const value."));
+    assert(std::is_object<_Up>::value);
+    assert(__any.type() == typeid(_Up));
+    auto __p = static_cast<_Up*>(__any._M_data);
+    if (__p)
+        return static_cast<_ValueType>(*__p);
+    throw std::bad_cast();
+}
+
+#endif /* AIDGE_ANY_H_ */
\ No newline at end of file

include/aidge/utils/CParameter.hpp

@@ -12,23 +12,34 @@
 #ifndef AIDGE_CPARAMETER_H_
 #define AIDGE_CPARAMETER_H_
 
-#include <assert.h>
 #include <map>
 #include <vector>
+#include <type_traits>
+#include <typeinfo>
+#include <assert.h>
+
+#include "aidge/utils/Any.hpp"
+
 
 namespace Aidge {
 
 ///\todo store also a fix-sized code that indicates the type
 ///\todo managing complex types or excluding non-trivial, non-aggregate types
-class CParameter
-{
+class CParameter {
 private:
-    template <typename T>
-    struct is_vector : std::false_type {};
-
-    template <typename T, typename Alloc>
-    struct is_vector<std::vector<T, Alloc>> : std::true_type {};
-
+    template<typename _ValueType>
+    inline _ValueType& any_cast_ref(const _any& __any)
+    {
+        using _Up =  std::remove_cv_t<std::remove_reference_t<_ValueType>>;
+        assert(((std::is_reference<_ValueType>::value || std::is_copy_constructible<_ValueType>::value) && "Template argument must be a reference or CopyConstructible type"));
+        assert((std::is_constructible<_ValueType, const _Up&>::value && "Template argument must be constructible from a const value."));
+        assert(std::is_object<_Up>::value);
+        assert(__any.type() == typeid(_Up));
+        if (_any::Manager<_Up>::access(&__any)) { // assess if _any object is empty
+            return *static_cast<_ValueType*>(_any::Manager<_Up>::access(&__any));
+        }
+        throw std::bad_cast();
+    }
 public:
     // not copyable, not movable
     CParameter(CParameter const &) = delete;
@@ -48,15 +59,16 @@ public:
      *  param buffer that will get invalid after the CParam death.
      * \note at() throws if the parameter does not exist, using find to test for parameter existance
      */
-    template<class T> T Get(std::string const i_ParamName) const
+    template<class T> T& Get(const std::string i_ParamName)
     {
-        assert(m_Params.find(i_ParamName) != m_Params.end());
-        assert(m_Types.find(i_ParamName) != m_Types.end());
-        assert(m_Params.at(i_ParamName) <= m_OffSet);
-        assert(typeid(T).name() == m_Types.at(i_ParamName));
-        return *reinterpret_cast<T *>(m_BeginBuffer + m_Params.at(i_ParamName));
+        return any_cast_ref<T>(m_Buffer[m_Params.at(i_ParamName)]);
     }
 
+    // template<class T> const T& Get(const std::string i_ParamName) const
+    // {
+    //     return any_cast<T>(m_Buffer[m_Params.at(i_ParamName)]);
+    // }
+
     ///\brief Add a parameter value, identified by its name
     ///\tparam T expected parameter type
     ///\param i_ParamName Parameter name
@@ -64,21 +76,15 @@ public:
     ///\todo Pass i_Value by ref if large or not trivial
     ///\bug If parameter already exists, its value is changed but written in the
     /// internal buffer in a new location (previous value is still in memory at its previous location)
-    template<class T> void Add(std::string const &i_ParamName, T const &i_Value)
+    template<class T> void Add(const std::string &i_ParamName, T&& i_Value)
     {
-        m_Buffer.resize(m_Buffer.size() + (sizeof(T) / sizeof(uint8_t)));
-        m_BeginBuffer = m_Buffer.data(); // Update buffer ptr in case of memory reordering
-        *reinterpret_cast<T *>(m_BeginBuffer + m_OffSet)
-            = i_Value; // Black-magic used to add anytype into the vector
-        m_Params[i_ParamName] = m_OffSet; // Copy pointer offset
-        m_OffSet += sizeof(T); // Increment offset
-
-        m_Types[i_ParamName] = typeid(i_Value).name();
+        m_Params[i_ParamName] = m_Buffer.size(); // Copy pointer offset
+        m_Buffer.push_back(_any(std::forward<T>(i_Value)));
     }
 
 
     std::string getParamType(std::string const &i_ParamName){
-        return m_Types[i_ParamName];
+        return m_Buffer[m_Params.at(i_ParamName)].type().name();
     }
 
     std::vector<std::string> getParametersName(){
@@ -91,23 +97,8 @@ public:
 private:
     std::map<std::string, std::size_t> m_Params; // { Param name : offset }
 
-    ///\brief Map to check type error
-    /* Note : i tried this : `std::map<std::string, std::type_info const *> mTypes;`
-    but looks like the type_ingo object was destroyed.
-    I am not a hugde fan of storing a string and making string comparison.
-    Maybe we can use a custom enum type (or is there a standard solution ?)
-    */
-    std::map<std::string, std::string> m_Types;
-
-    ///\brief All parameters values concatenated in raw binary form.
-    std::vector<uint8_t> m_Buffer = {};
-
-    ///\brief Starting address of the buffer
-    uint8_t *m_BeginBuffer = m_Buffer.data();
-
-    ///\brief Offset, in number of uint8_t, of the next parameter to write
-    std::size_t m_OffSet = 0;
-
+    ///\brief All raw pointers to parameters values concatenated. Use custom any class compatible with C++14.
+    std::vector<_any> m_Buffer = {};
 };
 
 }

src/graph/GraphView.cpp

@@ -33,13 +33,10 @@ Aidge::Connector Aidge::GraphView::operator()(
     (void)input; // avoid unused warning
   }
 
+  IOIndex_t inID = 0;
   for (const Connector &ctor : ctors) {
     assert((ctor.node() != nullptr) &&
            "Input Connector must be associated with a node");
-    (void)ctors; // avoid unused warning
-  }
-  IOIndex_t inID = 0;
-  for (const Connector &ctor : ctors) {
     ctor.node()->addChild(shared_from_this(), static_cast<std::size_t>(ctor.index()),
                           {inNode, inID++});
   }
@@ -197,7 +194,7 @@ void Aidge::GraphView::forwardDims() {
             {
               assert(!std::static_pointer_cast<Tensor>(nodePtr->getOperator()->getRawInput(i))->empty());
             }
-            
+
         }
     }
     // Compute dimensions of every node
@@ -537,7 +534,7 @@ bool Aidge::GraphView::replaceWith(std::set<std::shared_ptr<Node>> newNodes) {
   std::shared_ptr<Node> newInputNode;
   std::shared_ptr<Node> previousOutputNode;
   std::shared_ptr<Node> newOutputNode;
-  
+
   auto gNew = std::make_shared<GraphView>();
   gNew->add(newNodes, false);
 

unit_tests/operator/Test_GenericOperator.cpp

@@ -20,10 +20,10 @@ using namespace Aidge;
 TEST_CASE("[core/operators] GenericOp(add & get parameters)", "[Operator]") {
     SECTION("INT") {
         GenericOperator_Op Testop("TestOp", 1, 1, 1);
-        int value = 5;
         const char* key = "intParam";
-        Testop.addParameter(key, value);
-        REQUIRE(Testop.getParameter<int>(key) == value);
+        Testop.addParameter(key, int(5));
+        int registeredVal = Testop.getParameter<int>(key);
+        REQUIRE(registeredVal == 5);
     }
     SECTION("LONG") {
         GenericOperator_Op Testop("TestOp", 1, 1, 1);