From e4b9550efa5c12bda834351e1e737b255f85abdb Mon Sep 17 00:00:00 2001
From: Deukhoofd <deukhoofd@gmail.com>
Date: Tue, 2 Jun 2020 13:43:44 +0200
Subject: [PATCH] Make all individual scripts smart pointers.

---
 CInterface/Battling/ExecutingAttack.cpp       |  6 +--
 CInterface/Battling/TurnChoices.cpp           |  2 +-
 src/Battling/Flow/TurnHandler.cpp             |  1 -
 src/Battling/Models/Creature.cpp              |  5 +--
 src/Battling/Models/Creature.hpp              |  9 ++--
 src/Battling/Models/ExecutingAttack.hpp       |  9 ++--
 .../ScriptHandling/ScriptAggregator.hpp       |  2 +-
 src/Battling/ScriptHandling/ScriptWrapper.hpp | 10 ++---
 src/Battling/TurnChoices/AttackTurnChoice.hpp | 10 ++---
 .../ScriptTests/ScriptAggregatorTests.cpp     | 44 +++++++++----------
 .../ScriptTests/ScriptSourceTest.cpp          |  8 ++--
 11 files changed, 48 insertions(+), 58 deletions(-)

diff --git a/CInterface/Battling/ExecutingAttack.cpp b/CInterface/Battling/ExecutingAttack.cpp
index 86923c0..e68e141 100644
--- a/CInterface/Battling/ExecutingAttack.cpp
+++ b/CInterface/Battling/ExecutingAttack.cpp
@@ -5,9 +5,9 @@ using namespace CreatureLib::Battling;
 export uint8_t CreatureLib_ExecutingAttack_Construct(ExecutingAttack*& out, Creature* const* targets,
                                                      size_t targetCount, uint8_t numberHits, Creature* user,
                                                      LearnedAttack* attack, Script* script) {
-    Try(auto ls = ArbUt::List<ArbUt::BorrowedPtr<Creature>>(targetCount); for (size_t i = 0; i < targetCount; i++) {
-        ls.Append(targets[i]);
-    } out = new ExecutingAttack(ls, numberHits, user, attack, script);)
+    Try(auto ls = ArbUt::List<ArbUt::BorrowedPtr<Creature>>(targetCount);
+        for (size_t i = 0; i < targetCount; i++) { ls.Append(targets[i]); } auto s = std::unique_ptr<Script>(script);
+        out = new ExecutingAttack(ls, numberHits, user, attack, s);)
 }
 
 export void CreatureLib_ExecutingAttack_Destruct(ExecutingAttack* p) { delete p; }
diff --git a/CInterface/Battling/TurnChoices.cpp b/CInterface/Battling/TurnChoices.cpp
index 4a4fab2..51da88e 100644
--- a/CInterface/Battling/TurnChoices.cpp
+++ b/CInterface/Battling/TurnChoices.cpp
@@ -30,7 +30,7 @@ SIMPLE_GET_FUNC(AttackTurnChoice, GetKind, TurnChoiceKind)
 
 export uint8_t CreatureLib_BaseTurnChoice_GetPriority(int8_t& out, AttackTurnChoice* p) { Try(out = p->GetPriority()); }
 
-SIMPLE_GET_FUNC(AttackTurnChoice, GetAttackScript, Script*)
+SIMPLE_GET_FUNC_SMART_PTR(AttackTurnChoice, GetAttackScript, Script*)
 export uint8_t CreatureLib_BaseTurnChoice_GetTargetSideIndex(const AttackTurnChoice* p) {
     return p->GetTarget().GetSideIndex();
 }
diff --git a/src/Battling/Flow/TurnHandler.cpp b/src/Battling/Flow/TurnHandler.cpp
index 1a4ad74..c4b0d76 100644
--- a/src/Battling/Flow/TurnHandler.cpp
+++ b/src/Battling/Flow/TurnHandler.cpp
@@ -62,7 +62,6 @@ void TurnHandler::ExecuteAttackChoice(ArbUt::BorrowedPtr<AttackTurnChoice> choic
     ArbUt::List<ArbUt::BorrowedPtr<Creature>> targets = {target};
 
     auto attack = ExecutingAttack(targets, 1, choice->GetUser(), choice->GetAttack(), choice->GetAttackScript());
-    choice->MarkScriptAsTaken();
     bool prevented = false;
     HOOK_LOCAL(PreventAttack, attack, &attack, &prevented);
     if (prevented) {
diff --git a/src/Battling/Models/Creature.cpp b/src/Battling/Models/Creature.cpp
index c63c0a8..22a94e5 100644
--- a/src/Battling/Models/Creature.cpp
+++ b/src/Battling/Models/Creature.cpp
@@ -21,7 +21,7 @@ Battling::Creature::Creature(const BattleLibrary* library,
     AssertNotNull(species)
     AssertNotNull(variant)
 
-    _activeTalent = _library->LoadScript(ScriptCategory::Talent, GetActiveTalent());
+    _activeTalent = std::unique_ptr<Script>(_library->LoadScript(ScriptCategory::Talent, GetActiveTalent()));
     if (_nickname.empty()) {
         _nickname = species->GetName().std_str();
     }
@@ -142,9 +142,8 @@ void Battling::Creature::Heal(uint32_t amount, bool canRevive) {
 void Battling::Creature::OverrideActiveTalent(const ConstString& talent) {
     _hasOverridenTalent = true;
     _activeTalent->OnRemove();
-    delete _activeTalent;
     _overridenTalentName = talent;
-    _activeTalent = this->_library->LoadScript(ScriptCategory::Talent, talent);
+    _activeTalent.reset(this->_library->LoadScript(ScriptCategory::Talent, talent));
 }
 
 const ArbUt::List<uint8_t>& Battling::Creature::GetTypes() const noexcept {
diff --git a/src/Battling/Models/Creature.hpp b/src/Battling/Models/Creature.hpp
index 2498bc4..c639c6d 100644
--- a/src/Battling/Models/Creature.hpp
+++ b/src/Battling/Models/Creature.hpp
@@ -47,7 +47,7 @@ namespace CreatureLib::Battling {
 
         std::string _nickname = "";
         CreatureLib::Library::TalentIndex _talentIndex;
-        Script* _activeTalent = nullptr;
+        std::unique_ptr<Script> _activeTalent = nullptr;
 
         bool _hasOverridenTalent;
         ArbUt::CaseInsensitiveConstString _overridenTalentName = ""_cnc;
@@ -56,7 +56,7 @@ namespace CreatureLib::Battling {
         ArbUt::UniquePtrList<LearnedAttack> _attacks;
         bool _allowedExperienceGain;
 
-        Script* _status = nullptr;
+        std::unique_ptr<Script> _status = nullptr;
         ScriptSet _volatile = {};
 
     private:
@@ -70,10 +70,7 @@ namespace CreatureLib::Battling {
                  const Library::TalentIndex& talent, const std::vector<LearnedAttack*>& attacks,
                  bool allowedExperienceGain = true);
 
-        virtual ~Creature() {
-            delete _activeTalent;
-            delete _status;
-        };
+        virtual ~Creature() = default;
 
         virtual void Initialize() {
             RecalculateFlatStats();
diff --git a/src/Battling/Models/ExecutingAttack.hpp b/src/Battling/Models/ExecutingAttack.hpp
index b3b49e5..1366464 100644
--- a/src/Battling/Models/ExecutingAttack.hpp
+++ b/src/Battling/Models/ExecutingAttack.hpp
@@ -41,24 +41,25 @@ namespace CreatureLib::Battling {
         HitData* _hits;
         Creature* _user;
         ArbUt::BorrowedPtr<LearnedAttack> _attack;
-        Script* _script;
+        std::unique_ptr<Script> _script = nullptr;
 
     public:
         ExecutingAttack(const ArbUt::List<ArbUt::BorrowedPtr<Creature>>& targets, uint8_t numberHits, Creature* user,
-                        const ArbUt::BorrowedPtr<LearnedAttack>& attack, Script* script)
+                        const ArbUt::BorrowedPtr<LearnedAttack>& attack, const std::unique_ptr<Script>& script)
             : _targets(targets.Count()), _numberHits(numberHits), _hits(new HitData[targets.Count() * numberHits]),
-              _user(user), _attack(attack), _script(script) {
+              _user(user), _attack(attack) {
             AssertNotNull(user)
             AssertNotNull(attack)
             for (auto target : targets) {
                 _targets.Append(target);
             }
+            // Take ownership of the script of the attack choice, and give attack choice our initial nullptr.
+            _script.swap(const_cast<std::unique_ptr<Script>&>(script));
         }
         ExecutingAttack(const ExecutingAttack&) = delete;
         ExecutingAttack& operator=(const ExecutingAttack&) = delete;
 
         virtual ~ExecutingAttack() noexcept {
-            delete _script;
             delete[] _hits;
         };
 
diff --git a/src/Battling/ScriptHandling/ScriptAggregator.hpp b/src/Battling/ScriptHandling/ScriptAggregator.hpp
index b0ee820..1c0696b 100644
--- a/src/Battling/ScriptHandling/ScriptAggregator.hpp
+++ b/src/Battling/ScriptHandling/ScriptAggregator.hpp
@@ -42,7 +42,7 @@ namespace CreatureLib::Battling {
                 auto s = current.GetScript();
                 if (s == nullptr)
                     return nullptr;
-                return *s;
+                return (*s).get();
             } else {
                 auto& set = *current.GetScriptSet()->GetIterator();
                 auto count = set.Count();
diff --git a/src/Battling/ScriptHandling/ScriptWrapper.hpp b/src/Battling/ScriptHandling/ScriptWrapper.hpp
index 3abd943..cacd2b8 100644
--- a/src/Battling/ScriptHandling/ScriptWrapper.hpp
+++ b/src/Battling/ScriptHandling/ScriptWrapper.hpp
@@ -1,7 +1,7 @@
 #ifndef CREATURELIB_SCRIPTWRAPPER_HPP
 #define CREATURELIB_SCRIPTWRAPPER_HPP
 
-#include <variant>
+#include <memory>
 #include "Script.hpp"
 #include "ScriptSet.hpp"
 
@@ -10,20 +10,20 @@ namespace CreatureLib::Battling {
         bool _isSet;
 
         union {
-            Script* const* _script;
+            std::unique_ptr<Script> const* _script;
             const ScriptSet* _scriptSet;
         };
 
-        ScriptWrapper(Script** s, bool isSet) : _isSet(isSet), _script(s){};
+        ScriptWrapper(std::unique_ptr<Script>* s, bool isSet) : _isSet(isSet), _script(s){};
         ScriptWrapper(ScriptSet* s, bool isSet) : _isSet(isSet), _scriptSet(s){};
 
     public:
-        static inline ScriptWrapper FromScript(Script** s) { return ScriptWrapper(s, false); }
+        static inline ScriptWrapper FromScript(std::unique_ptr<Script>* s) { return ScriptWrapper(s, false); }
         static inline ScriptWrapper FromSet(ScriptSet* s) { return ScriptWrapper(s, true); }
 
         bool IsSet() const { return _isSet; }
 
-        inline Script* const* GetScript() const { return _script; }
+        inline const std::unique_ptr<Script>* GetScript() const { return _script; }
         inline const ScriptSet* GetScriptSet() const { return _scriptSet; }
     };
 }
diff --git a/src/Battling/TurnChoices/AttackTurnChoice.hpp b/src/Battling/TurnChoices/AttackTurnChoice.hpp
index 6ecdfdc..5eeee16 100644
--- a/src/Battling/TurnChoices/AttackTurnChoice.hpp
+++ b/src/Battling/TurnChoices/AttackTurnChoice.hpp
@@ -11,7 +11,7 @@ namespace CreatureLib::Battling {
     class AttackTurnChoice : public BaseTurnChoice {
         ArbUt::BorrowedPtr<LearnedAttack> _attack;
         CreatureIndex _target;
-        Script* _attackScript = nullptr;
+        std::unique_ptr<Script> _attackScript = nullptr;
         bool _scriptIsTaken = false;
 
         void ResolveScript() {
@@ -25,7 +25,8 @@ namespace CreatureLib::Battling {
                 if (_attack->GetAttack()->HasSecondaryEffect()) {
                     auto library = battle->GetLibrary();
                     auto& effect = _attack->GetAttack()->GetSecondaryEffect();
-                    _attackScript = library->LoadScript(ScriptCategory::Attack, effect->GetEffectName());
+                    _attackScript =
+                        std::unique_ptr<Script>(library->LoadScript(ScriptCategory::Attack, effect->GetEffectName()));
                     if (_attackScript != nullptr) {
                         _attackScript->OnInitialize(effect->GetParameters());
                     }
@@ -45,7 +46,6 @@ namespace CreatureLib::Battling {
 
         ~AttackTurnChoice() {
             if (!_scriptIsTaken) {
-                delete _attackScript;
             }
         }
 
@@ -58,11 +58,9 @@ namespace CreatureLib::Battling {
             return _attack->GetAttack()->GetPriority();
         }
 
-        void MarkScriptAsTaken() { _scriptIsTaken = true; }
-
         const CreatureIndex& GetTarget() const noexcept { return _target; }
 
-        Script* GetAttackScript() const noexcept { return _attackScript; }
+        const std::unique_ptr<Script>& GetAttackScript() const noexcept { return _attackScript; }
         size_t ScriptCount() const override { return 1 + GetUser()->ScriptCount(); }
 
     protected:
diff --git a/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp b/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp
index 74ca08f..79f5ee5 100644
--- a/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp
+++ b/tests/BattleTests/ScriptTests/ScriptAggregatorTests.cpp
@@ -19,9 +19,10 @@ public:
 };
 
 TEST_CASE("Script Aggregator properly iterates containing script.", "[Battling, Scripting]") {
-    Script* script = new TestScript("test");
+    auto script = std::make_unique<TestScript>("test");
     auto ran = 0;
-    auto vec = ArbUt::List<ScriptWrapper>{ScriptWrapper::FromScript(&script)};
+    auto vec =
+        ArbUt::List<ScriptWrapper>{ScriptWrapper::FromScript(reinterpret_cast<std::unique_ptr<Script>*>(&script))};
     auto aggr = ScriptAggregator(vec);
     while (aggr.HasNext()) {
         auto next = aggr.GetNext();
@@ -29,16 +30,17 @@ TEST_CASE("Script Aggregator properly iterates containing script.", "[Battling,
         dynamic_cast<TestScript*>(next)->TestMethod(ran);
     }
     CHECK(ran == 1);
-    delete script;
 }
 
 TEST_CASE("Script Aggregator properly iterates multiple scripts.", "[Battling, Scripting]") {
-    Script* script = new TestScript("test");
-    Script* script2 = new TestScript("test2");
-    Script* script3 = new TestScript("test3");
+    auto script = std::make_unique<TestScript>("test");
+    auto script2 = std::make_unique<TestScript>("test2");
+    auto script3 = std::make_unique<TestScript>("test3");
     auto ran = 0;
-    auto vec = ArbUt::List<ScriptWrapper>{ScriptWrapper::FromScript(&script), ScriptWrapper::FromScript(&script2),
-                                          ScriptWrapper::FromScript(&script3)};
+    auto vec =
+        ArbUt::List<ScriptWrapper>{ScriptWrapper::FromScript(reinterpret_cast<std::unique_ptr<Script>*>(&script)),
+                                   ScriptWrapper::FromScript(reinterpret_cast<std::unique_ptr<Script>*>(&script2)),
+                                   ScriptWrapper::FromScript(reinterpret_cast<std::unique_ptr<Script>*>(&script3))};
     auto aggr = ScriptAggregator(vec);
     while (aggr.HasNext()) {
         auto next = aggr.GetNext();
@@ -46,9 +48,6 @@ TEST_CASE("Script Aggregator properly iterates multiple scripts.", "[Battling, S
         dynamic_cast<TestScript*>(next)->TestMethod(ran);
     }
     CHECK(ran == 3);
-    delete script;
-    delete script2;
-    delete script3;
 }
 
 TEST_CASE("Script Aggregator properly iterates Script Set.", "[Battling, Scripting]") {
@@ -71,14 +70,15 @@ TEST_CASE("Script Aggregator properly iterates Script Set.", "[Battling, Scripti
 }
 
 TEST_CASE("Script Aggregator properly iterates data of Script Set and Script.", "[Battling, Scripting]") {
-    Script* script = new TestScript("test");
+    auto script = std::make_unique<TestScript>("test");
     Script* script2 = new TestScript("test2");
     Script* script3 = new TestScript("test3");
     auto ran = 0;
     auto set = ScriptSet();
     set.Add(script2);
     set.Add(script3);
-    auto vec = ArbUt::List<ScriptWrapper>{ScriptWrapper::FromSet(&set), ScriptWrapper::FromScript(&script)};
+    auto vec = ArbUt::List<ScriptWrapper>{
+        ScriptWrapper::FromSet(&set), ScriptWrapper::FromScript(reinterpret_cast<std::unique_ptr<Script>*>(&script))};
     auto aggr = ScriptAggregator(vec);
     while (aggr.HasNext()) {
         auto next = aggr.GetNextNotNull();
@@ -86,18 +86,18 @@ TEST_CASE("Script Aggregator properly iterates data of Script Set and Script.",
         dynamic_cast<TestScript*>(next)->TestMethod(ran);
     }
     CHECK(ran == 3);
-    delete script;
 }
 
 TEST_CASE("Script Aggregator properly iterates data of Script and Script Set.", "[Battling, Scripting]") {
-    Script* script = new TestScript("test");
+    auto script = std::make_unique<TestScript>("test");
     Script* script2 = new TestScript("test2");
     Script* script3 = new TestScript("test3");
     auto ran = 0;
     auto set = ScriptSet();
     set.Add(script2);
     set.Add(script3);
-    auto vec = ArbUt::List<ScriptWrapper>{ScriptWrapper::FromScript(&script), ScriptWrapper::FromSet(&set)};
+    auto vec = ArbUt::List<ScriptWrapper>{
+        ScriptWrapper::FromScript(reinterpret_cast<std::unique_ptr<Script>*>(&script)), ScriptWrapper::FromSet(&set)};
     auto aggr = ScriptAggregator(vec);
     while (aggr.HasNext()) {
         auto next = aggr.GetNextNotNull();
@@ -105,20 +105,20 @@ TEST_CASE("Script Aggregator properly iterates data of Script and Script Set.",
         dynamic_cast<TestScript*>(next)->TestMethod(ran);
     }
     CHECK(ran == 3);
-    delete script;
 }
 
 TEST_CASE("Script Aggregator properly iterates data of Script, Script Set and Script.", "[Battling, Scripting]") {
-    Script* script = new TestScript("test");
+    auto script = std::make_unique<TestScript>("test");
     Script* script2 = new TestScript("test2");
     Script* script3 = new TestScript("test3");
-    Script* script4 = new TestScript("test4");
+    auto script4 = std::make_unique<TestScript>("test4");
     auto ran = 0;
     auto set = ScriptSet();
     set.Add(script2);
     set.Add(script3);
-    auto vec = ArbUt::List<ScriptWrapper>{ScriptWrapper::FromScript(&script), ScriptWrapper::FromSet(&set),
-                                          ScriptWrapper::FromScript(&script4)};
+    auto vec = ArbUt::List<ScriptWrapper>{
+        ScriptWrapper::FromScript(reinterpret_cast<std::unique_ptr<Script>*>(&script)), ScriptWrapper::FromSet(&set),
+        ScriptWrapper::FromScript(reinterpret_cast<std::unique_ptr<Script>*>(&script4))};
     auto aggr = ScriptAggregator(vec);
     while (aggr.HasNext()) {
         auto next = aggr.GetNextNotNull();
@@ -126,8 +126,6 @@ TEST_CASE("Script Aggregator properly iterates data of Script, Script Set and Sc
         dynamic_cast<TestScript*>(next)->TestMethod(ran);
     }
     CHECK(ran == 4);
-    delete script;
-    delete script4;
 }
 
 TEST_CASE("Script Aggregator properly iterates when empty.", "[Battling, Scripting]") {
diff --git a/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp b/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp
index 4ee6d29..6b5707a 100644
--- a/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp
+++ b/tests/BattleTests/ScriptTests/ScriptSourceTest.cpp
@@ -21,7 +21,7 @@ public:
 
 class ScriptSourceWithScriptPtr : public ScriptSource {
 public:
-    Script* ScriptPtr = nullptr;
+    std::unique_ptr<Script> ScriptPtr = nullptr;
 
 protected:
     size_t ScriptCount() const override { return 1; }
@@ -50,11 +50,10 @@ TEST_CASE("Script source with unset script ptr.", "[Battling, Scripting]") {
 
 TEST_CASE("Script source with script ptr being set.", "[Battling, Scripting]") {
     auto source = ScriptSourceWithScriptPtr();
-    source.ScriptPtr = new TestScript("foobar");
+    source.ScriptPtr = std::make_unique<TestScript>("foobar");
     auto scripts = source.GetScriptIterator();
     auto first = scripts.GetNext();
     CHECK(first != nullptr);
-    delete source.ScriptPtr;
 }
 
 TEST_CASE("Script source with script ptr being set after first iteration.", "[Battling, Scripting]") {
@@ -62,11 +61,10 @@ TEST_CASE("Script source with script ptr being set after first iteration.", "[Ba
     auto scripts = source.GetScriptIterator();
     auto first = scripts.GetNext();
     CHECK(first == nullptr);
-    source.ScriptPtr = new TestScript("foobar");
+    source.ScriptPtr = std::make_unique<TestScript>("foobar");
     scripts = source.GetScriptIterator();
     first = scripts.GetNext();
     CHECK(first != nullptr);
-    delete source.ScriptPtr;
 }
 
 TEST_CASE("Script source with empty script set.", "[Battling, Scripting]") {