From 6d9b55ebc07458e422f6e2d10fce05ea34e65f12 Mon Sep 17 00:00:00 2001
From: Ryan Blue <ryanzblue@gmail.com>
Date: Tue, 22 Oct 2024 22:44:38 -0400
Subject: [PATCH] Fix issues found in testing

- move constructor now invalidates old state
- SetText gets a reference of the set, not a copy
---
 wpilibc/src/main/native/cpp/Alert.cpp       | 23 ++++++++++++++++++++-
 wpilibc/src/main/native/include/frc/Alert.h | 15 ++++++++++----
 2 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/wpilibc/src/main/native/cpp/Alert.cpp b/wpilibc/src/main/native/cpp/Alert.cpp
index 628c9373439..0394f8a14ac 100644
--- a/wpilibc/src/main/native/cpp/Alert.cpp
+++ b/wpilibc/src/main/native/cpp/Alert.cpp
@@ -27,6 +27,27 @@ Alert::Alert(std::string_view text, AlertType type)
 Alert::Alert(std::string_view group, std::string_view text, AlertType type)
     : m_type(type), m_text(text), m_group{&GetGroupSendable(group)} {}
 
+Alert::Alert(Alert&& other)
+    : m_type{other.m_type},
+      m_text{std::move(other.m_text)},
+      m_group{std::exchange(other.m_group, nullptr)},
+      m_active{std::exchange(other.m_active, false)},
+      m_activeStartTime{other.m_activeStartTime} {}
+
+Alert& Alert::operator=(Alert&& other) {
+  if (&other != this) {
+    // We want to destroy current state after the move is done
+    Alert tmp{std::move(*this)};
+    // Now, swap moved-from state with other state
+    std::swap(m_type, other.m_type);
+    std::swap(m_text, other.m_text);
+    std::swap(m_group, other.m_group);
+    std::swap(m_active, other.m_active);
+    std::swap(m_activeStartTime, other.m_activeStartTime);
+  }
+  return *this;
+}
+
 Alert::~Alert() {
   Set(false);
 }
@@ -54,7 +75,7 @@ void Alert::SetText(std::string_view text) {
   m_text = text;
 
   if (m_active) {
-    auto set = m_group->GetSetForType(m_type);
+    auto& set = m_group->GetSetForType(m_type);
     auto iter = set.find({m_activeStartTime, oldText});
     auto hint = set.erase(iter);
     set.emplace_hint(hint, m_activeStartTime, m_text);
diff --git a/wpilibc/src/main/native/include/frc/Alert.h b/wpilibc/src/main/native/include/frc/Alert.h
index 4d43edd5e58..a38901befc8 100644
--- a/wpilibc/src/main/native/include/frc/Alert.h
+++ b/wpilibc/src/main/native/include/frc/Alert.h
@@ -91,8 +91,8 @@ class Alert {
    */
   Alert(std::string_view group, std::string_view text, AlertType type);
 
-  Alert(Alert&&) = default;
-  Alert& operator=(Alert&&) = default;
+  Alert(Alert&&);
+  Alert& operator=(Alert&&);
 
   ~Alert();
 
@@ -130,6 +130,8 @@ class Alert {
     void InitSendable(nt::NTSendableBuilder& builder) override;
 
     std::set<PublishedAlert>& GetSetForType(AlertType type);
+    // todo: not sure why i needed this, maybe when i was testing different
+    // container types?
     const std::set<PublishedAlert>& GetSetForType(AlertType type) const;
 
    private:
@@ -139,11 +141,16 @@ class Alert {
 
   AlertType m_type;
   std::string m_text;
-  SendableAlerts* m_group;
-
+  SendableAlerts* m_group;  // could replace with pointer to type set
   bool m_active = false;
   uint64_t m_activeStartTime;
 
+  /**
+   * Returns the SendableAlerts for a given group, initializing and publishing
+   * if it does not already exist.
+   * @param group the group name
+   * @return the SendableAlerts for the group
+   */
   static SendableAlerts& GetGroupSendable(std::string_view group);
 };