From b6748be27575417b5d8bf6a330aeac6385e38dd6 Mon Sep 17 00:00:00 2001 From: anutosh491 Date: Tue, 18 Jun 2024 14:05:02 +0530 Subject: [PATCH 1/3] Refactored register_preamble --- include/xeus-cpp/xholder.hpp | 6 +++--- include/xeus-cpp/xmanager.hpp | 8 ++++---- include/xeus-cpp/xpreamble.hpp | 2 +- src/xholder.cpp | 14 ++++++-------- src/xinspect.hpp | 4 ++-- src/xinterpreter.cpp | 7 +++---- src/xsystem.hpp | 4 ++-- test/test_interpreter.cpp | 25 ++++++++++++------------- 8 files changed, 33 insertions(+), 37 deletions(-) diff --git a/include/xeus-cpp/xholder.hpp b/include/xeus-cpp/xholder.hpp index 4f7a38d6..450c45a2 100644 --- 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 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 holder); void swap(xholder_preamble& rhs) { @@ -53,7 +53,7 @@ namespace xcpp private: - xpreamble* p_holder; + std::unique_ptr p_holder; }; } #endif diff --git a/include/xeus-cpp/xmanager.hpp b/include/xeus-cpp/xmanager.hpp index 483c0632..afa99fa4 100644 --- a/include/xeus-cpp/xmanager.hpp +++ b/include/xeus-cpp/xmanager.hpp @@ -31,9 +31,9 @@ namespace xcpp std::map preamble; template - void register_preamble(const std::string& name, preamble_type* pre) + void register_preamble(const std::string& name, std::unique_ptr 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 + virtual std::unique_ptr clone() const override { - return new xmagics_manager(*this); + return std::make_unique(*this); } private: diff --git a/include/xeus-cpp/xpreamble.hpp b/include/xeus-cpp/xpreamble.hpp index 9d6b66ea..60055578 100644 --- 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 clone() const = 0; virtual ~xpreamble(){}; }; } diff --git a/src/xholder.cpp b/src/xholder.cpp index 44938564..6a4ad768 100644 --- 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 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 holder) { - delete p_holder; - p_holder = holder; + p_holder = std::move(holder); return *this; } diff --git a/src/xinspect.hpp b/src/xinspect.hpp index 8ee760de..663d258d 100644 --- 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 + virtual std::unique_ptr clone() const override { - return new xintrospection(*this); + return std::make_unique(*this); } }; diff --git a/src/xinterpreter.cpp b/src/xinterpreter.cpp index 121b90a6..e715d457 100644 --- 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()); + preamble_manager.register_preamble("magics", std::make_unique()); + preamble_manager.register_preamble("shell", std::make_unique()); //NOLINTEND(cppcoreguidelines-owning-memory) } diff --git a/src/xsystem.hpp b/src/xsystem.hpp index 99d67b1e..cd52ca2e 100644 --- a/src/xsystem.hpp +++ b/src/xsystem.hpp @@ -67,9 +67,9 @@ namespace xcpp } } - virtual xpreamble* clone() const override + virtual std::unique_ptr clone() const override { - return new xsystem(*this); + return std::make_unique(*this); } }; } diff --git a/test/test_interpreter.cpp b/test/test_interpreter.cpp index a53a9042..028b7553 100644 --- 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 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 clone = manager.clone(); - REQUIRE(dynamic_cast(clone) != nullptr); - - delete clone; + REQUIRE(dynamic_cast(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 magics = std::make_unique(); + auto* raw_ptr = magics.get(); + manager.register_preamble(name, std::move(magics)); xcpp::xholder_preamble& result = manager.operator[](name); - REQUIRE(&(result.get_cast()) == magics); + REQUIRE(&(result.get_cast()) == raw_ptr); } } @@ -642,7 +641,7 @@ TEST_SUITE("xsystem_clone") { xcpp::xsystem system; - xcpp::xpreamble* clone = system.clone(); + std::unique_ptr 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 clone = system.clone(); - REQUIRE(dynamic_cast(clone) != nullptr); + REQUIRE(dynamic_cast(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()); preamble_manager["magics"].get_cast().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()); preamble_manager["magics"].get_cast().register_magic("magic1", MyMagicLine()); From f1d194165e28498a07d4d8c6ed08e659eadb71b6 Mon Sep 17 00:00:00 2001 From: anutosh491 Date: Tue, 18 Jun 2024 14:20:47 +0530 Subject: [PATCH 2/3] Clang tidy suggestions --- include/xeus-cpp/xmanager.hpp | 2 +- src/xinspect.hpp | 2 +- src/xsystem.hpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/xeus-cpp/xmanager.hpp b/include/xeus-cpp/xmanager.hpp index afa99fa4..32243ae5 100644 --- a/include/xeus-cpp/xmanager.hpp +++ b/include/xeus-cpp/xmanager.hpp @@ -186,7 +186,7 @@ namespace xcpp } } - virtual std::unique_ptr clone() const override + std::unique_ptr clone() const override { return std::make_unique(*this); } diff --git a/src/xinspect.hpp b/src/xinspect.hpp index 663d258d..dc5efa4e 100644 --- a/src/xinspect.hpp +++ b/src/xinspect.hpp @@ -279,7 +279,7 @@ namespace xcpp inspect(to_inspect[1], kernel_res); } - virtual std::unique_ptr clone() const override + std::unique_ptr clone() const override { return std::make_unique(*this); } diff --git a/src/xsystem.hpp b/src/xsystem.hpp index cd52ca2e..126c8773 100644 --- a/src/xsystem.hpp +++ b/src/xsystem.hpp @@ -67,7 +67,7 @@ namespace xcpp } } - virtual std::unique_ptr clone() const override + std::unique_ptr clone() const override { return std::make_unique(*this); } From 911a7a5a23ab05af4102585c32a6265cb3bf4553 Mon Sep 17 00:00:00 2001 From: anutosh491 Date: Tue, 18 Jun 2024 15:17:16 +0530 Subject: [PATCH 3/3] Added suggestions --- include/xeus-cpp/xmanager.hpp | 2 +- src/xinspect.hpp | 2 +- src/xsystem.hpp | 2 +- test/test_interpreter.cpp | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/include/xeus-cpp/xmanager.hpp b/include/xeus-cpp/xmanager.hpp index 32243ae5..3292ce39 100644 --- a/include/xeus-cpp/xmanager.hpp +++ b/include/xeus-cpp/xmanager.hpp @@ -186,7 +186,7 @@ namespace xcpp } } - std::unique_ptr clone() const override + [[nodiscard]] std::unique_ptr clone() const override { return std::make_unique(*this); } diff --git a/src/xinspect.hpp b/src/xinspect.hpp index dc5efa4e..bdc4a13e 100644 --- a/src/xinspect.hpp +++ b/src/xinspect.hpp @@ -279,7 +279,7 @@ namespace xcpp inspect(to_inspect[1], kernel_res); } - std::unique_ptr clone() const override + [[nodiscard]] std::unique_ptr clone() const override { return std::make_unique(*this); } diff --git a/src/xsystem.hpp b/src/xsystem.hpp index 126c8773..159dbeb9 100644 --- a/src/xsystem.hpp +++ b/src/xsystem.hpp @@ -67,7 +67,7 @@ namespace xcpp } } - std::unique_ptr clone() const override + [[nodiscard]] std::unique_ptr clone() const override { return std::make_unique(*this); } diff --git a/test/test_interpreter.cpp b/test/test_interpreter.cpp index 028b7553..364e1fd7 100644 --- a/test/test_interpreter.cpp +++ b/test/test_interpreter.cpp @@ -421,7 +421,7 @@ TEST_SUITE("clone_magics_manager") std::unique_ptr clone = manager.clone(); - REQUIRE(dynamic_cast(clone.get()) != nullptr); + REQUIRE(clone.get() != nullptr); } } @@ -652,7 +652,7 @@ TEST_SUITE("xsystem_clone") std::unique_ptr clone = system.clone(); - REQUIRE(dynamic_cast(clone.get()) != nullptr); + REQUIRE(clone.get() != nullptr); } }