Refactored register_preamble (#149)

compiler-research · Jun 18, 2024 · 24c9a41 · 24c9a41
1 parent 426dd34
commit 24c9a41
Show file tree

Hide file tree

Showing 8 changed files with 33 additions and 37 deletions.
diff --git a/include/xeus-cpp/xholder.hpp b/include/xeus-cpp/xholder.hpp
@@ -30,12 +30,12 @@ namespace xcpp
         ~xholder_preamble();
         xholder_preamble(const xholder_preamble& rhs);
         xholder_preamble(xholder_preamble&& rhs);
-        xholder_preamble(xpreamble* holder);
+        xholder_preamble(std::unique_ptr<xpreamble> holder);
 
         xholder_preamble& operator=(const xholder_preamble& rhs);
         xholder_preamble& operator=(xholder_preamble&& rhs);
 
-        xholder_preamble& operator=(xpreamble* holder);
+        xholder_preamble& operator=(std::unique_ptr<xpreamble> holder);
 
         void swap(xholder_preamble& rhs)
         {
@@ -53,7 +53,7 @@ namespace xcpp
 
     private:
 
-        xpreamble* p_holder;
+        std::unique_ptr<xpreamble> p_holder;
     };
 }
 #endif
diff --git a/include/xeus-cpp/xmanager.hpp b/include/xeus-cpp/xmanager.hpp
@@ -31,9 +31,9 @@ namespace xcpp
         std::map<std::string, xholder_preamble> preamble;
 
         template <typename preamble_type>
-        void register_preamble(const std::string& name, preamble_type* pre)
+        void register_preamble(const std::string& name, std::unique_ptr<preamble_type> pre)
         {
-            preamble[name] = xholder_preamble(pre);
+            preamble[name] = xholder_preamble(std::move(pre));
         }
 
         void unregister_preamble(const std::string& name)
@@ -186,9 +186,9 @@ namespace xcpp
             }
         }
 
-        virtual xpreamble* clone() const override
+        [[nodiscard]] std::unique_ptr<xpreamble> clone() const override
         {
-            return new xmagics_manager(*this);
+            return std::make_unique<xmagics_manager>(*this);
         }
 
     private:

diff --git a/include/xeus-cpp/xpreamble.hpp b/include/xeus-cpp/xpreamble.hpp
@@ -30,7 +30,7 @@ namespace xcpp
         }
 
         virtual void apply(const std::string& s, nl::json& kernel_res) = 0;
-        virtual xpreamble* clone() const = 0;
+        virtual std::unique_ptr<xpreamble> clone() const = 0;
         virtual ~xpreamble(){};
     };
 }

diff --git a/src/xholder.cpp b/src/xholder.cpp
@@ -24,23 +24,22 @@ namespace xcpp
     {
     }
 
-    xholder_preamble::xholder_preamble(xpreamble* holder)
-        : p_holder(holder)
+    xholder_preamble::xholder_preamble(std::unique_ptr<xpreamble> holder)
+        : p_holder(std::move(holder))
     {
     }
 
     xholder_preamble::~xholder_preamble()
     {
-        delete p_holder;
     }
 
     xholder_preamble::xholder_preamble(const xholder_preamble& rhs)
-        : p_holder(rhs.p_holder ? rhs.p_holder->clone() : nullptr)
+        : p_holder(rhs.p_holder ? std::move(rhs.p_holder->clone()) : nullptr)
     {
     }
 
     xholder_preamble::xholder_preamble(xholder_preamble&& rhs)
-        : p_holder(rhs.p_holder)
+        : p_holder(std::move(rhs.p_holder))
     {
         rhs.p_holder = nullptr;
     }
@@ -59,10 +58,9 @@ namespace xcpp
         return *this;
     }
 
-    xholder_preamble& xholder_preamble::operator=(xpreamble* holder)
+    xholder_preamble& xholder_preamble::operator=(std::unique_ptr<xpreamble> holder)
     {
-        delete p_holder;
-        p_holder = holder;
+        p_holder = std::move(holder);
         return *this;
     }
 

diff --git a/src/xinspect.hpp b/src/xinspect.hpp
@@ -279,9 +279,9 @@ namespace xcpp
             inspect(to_inspect[1], kernel_res);
         }
 
-        virtual xpreamble* clone() const override
+        [[nodiscard]] std::unique_ptr<xpreamble> clone() const override
         {
-            return new xintrospection(*this);
+            return std::make_unique<xintrospection>(*this);
         }
     };
 

diff --git a/src/xinterpreter.cpp b/src/xinterpreter.cpp
@@ -391,11 +391,10 @@ __get_cxx_version ()
 
     void interpreter::init_preamble()
     {
-        // FIXME: Make register_preamble take a unique_ptr.
         //NOLINTBEGIN(cppcoreguidelines-owning-memory)
-        preamble_manager.register_preamble("introspection", new xintrospection());
-        preamble_manager.register_preamble("magics", new xmagics_manager());
-        preamble_manager.register_preamble("shell", new xsystem());
+        preamble_manager.register_preamble("introspection", std::make_unique<xintrospection>());
+        preamble_manager.register_preamble("magics", std::make_unique<xmagics_manager>());
+        preamble_manager.register_preamble("shell", std::make_unique<xsystem>());
         //NOLINTEND(cppcoreguidelines-owning-memory)
     }
 

diff --git a/src/xsystem.hpp b/src/xsystem.hpp
@@ -67,9 +67,9 @@ namespace xcpp
             }
         }
 
-        virtual xpreamble* clone() const override
+        [[nodiscard]] std::unique_ptr<xpreamble> clone() const override
         {
-            return new xsystem(*this);
+            return std::make_unique<xsystem>(*this);
         }
     };
 }

diff --git a/test/test_interpreter.cpp b/test/test_interpreter.cpp
@@ -407,7 +407,7 @@ TEST_SUITE("clone_magics_manager")
     {
         xcpp::xmagics_manager manager;
 
-        xcpp::xpreamble* clone = manager.clone();
+        std::unique_ptr<xcpp::xpreamble> clone = manager.clone();
 
         REQUIRE(clone != nullptr);
     }
@@ -419,11 +419,9 @@ TEST_SUITE("clone_magics_manager")
     {
         xcpp::xmagics_manager manager;
 
-        xcpp::xpreamble* clone = manager.clone();
+        std::unique_ptr<xcpp::xpreamble> clone = manager.clone();
 
-        REQUIRE(dynamic_cast<xcpp::xmagics_manager*>(clone) != nullptr);
-
-        delete clone;
+        REQUIRE(clone.get() != nullptr);
     }
 }
 
@@ -436,12 +434,13 @@ TEST_SUITE("xpreamble_manager_operator")
     {
         std::string name = "test";
         xcpp::xpreamble_manager manager;
-        xcpp::xmagics_manager* magics = new xcpp::xmagics_manager();
-        manager.register_preamble(name, magics);
+        std::unique_ptr<xcpp::xmagics_manager> magics = std::make_unique<xcpp::xmagics_manager>();
+        auto* raw_ptr = magics.get();
+        manager.register_preamble(name, std::move(magics));
 
         xcpp::xholder_preamble& result = manager.operator[](name);
 
-        REQUIRE(&(result.get_cast<xcpp::xmagics_manager>()) == magics);
+        REQUIRE(&(result.get_cast<xcpp::xmagics_manager>()) == raw_ptr);
     }
 }
 
@@ -642,7 +641,7 @@ TEST_SUITE("xsystem_clone")
     {
         xcpp::xsystem system;
 
-        xcpp::xpreamble* clone = system.clone();
+        std::unique_ptr<xcpp::xpreamble> clone = system.clone();
 
         REQUIRE(clone != nullptr);
     }
@@ -651,9 +650,9 @@ TEST_SUITE("xsystem_clone")
     {
         xcpp::xsystem system;
 
-        xcpp::xpreamble* clone = system.clone();
+        std::unique_ptr<xcpp::xpreamble> clone = system.clone();
 
-        REQUIRE(dynamic_cast<xcpp::xsystem*>(clone) != nullptr);
+        REQUIRE(clone.get() != nullptr);
 
     }
 }
@@ -727,7 +726,7 @@ TEST_SUITE("xmagics_apply"){
 
         xcpp::xpreamble_manager preamble_manager;
 
-        preamble_manager.register_preamble("magics", new xcpp::xmagics_manager());
+        preamble_manager.register_preamble("magics", std::make_unique<xcpp::xmagics_manager>());
 
         preamble_manager["magics"].get_cast<xcpp::xmagics_manager>().register_magic("magic2", MyMagicCell());
 
@@ -742,7 +741,7 @@ TEST_SUITE("xmagics_apply"){
 
         xcpp::xpreamble_manager preamble_manager;
 
-        preamble_manager.register_preamble("magics", new xcpp::xmagics_manager());
+        preamble_manager.register_preamble("magics", std::make_unique<xcpp::xmagics_manager>());
 
         preamble_manager["magics"].get_cast<xcpp::xmagics_manager>().register_magic("magic1", MyMagicLine());