misc cleanup and bug fixes

src/CMakeLists.txt

@@ -247,10 +247,10 @@ set(mmapper_SRCS
     mainwindow/AudioVolumeSlider.h
     mainwindow/MapZoomSlider.cpp
     mainwindow/MapZoomSlider.h
-    mainwindow/UpdateDialog.cpp
-    mainwindow/UpdateDialog.h
     mainwindow/ThemeManager.cpp
     mainwindow/ThemeManager.h
+    mainwindow/UpdateDialog.cpp
+    mainwindow/UpdateDialog.h
     mainwindow/aboutdialog.cpp
     mainwindow/aboutdialog.h
     mainwindow/findroomsdlg.cpp
@@ -592,9 +592,9 @@ set(mmapper_SRCS
     syntax/Value.h
     timers/CTimers.cpp
     timers/CTimers.h
+    timers/TimerDelegate.cpp
     timers/TimerModel.cpp
     timers/TimerModel.h
-    timers/TimerDelegate.cpp
     timers/TimerDelegate.h
     timers/TimerWidget.cpp
     timers/TimerWidget.h

src/clock/mumeclockwidget.cpp

@@ -28,20 +28,22 @@ MumeClockWidget::MumeClockWidget(GameObserver &observer, MumeClock &clock, QWidg
     assert(testAttribute(Qt::WA_Hover));
 
     observer.sig2_timeOfDayChanged.connect(m_lifetime,
-                                           [this](MumeTimeEnum time) { updateTime(time); });
+                                           [this](const MumeTimeEnum time) { updateTime(time); });
     observer.sig2_moonPhaseChanged.connect(m_lifetime, [this](MumeMoonPhaseEnum phase) {
         updateMoonPhase(phase);
     });
     observer.sig2_moonVisibilityChanged.connect(m_lifetime,
-                                                [this](MumeMoonVisibilityEnum visibility) {
+                                                [this](const MumeMoonVisibilityEnum visibility) {
                                                     updateMoonVisibility(visibility);
                                                 });
-    observer.sig2_seasonChanged.connect(m_lifetime,
-                                        [this](MumeSeasonEnum season) { updateSeason(season); });
-    observer.sig2_weatherChanged.connect(m_lifetime, [this](PromptWeatherEnum weather) {
+    observer.sig2_seasonChanged.connect(m_lifetime, [this](const MumeSeasonEnum season) {
+        updateSeason(season);
+    });
+    observer.sig2_weatherChanged.connect(m_lifetime, [this](const PromptWeatherEnum weather) {
         updateWeather(weather);
     });
-    observer.sig2_fogChanged.connect(m_lifetime, [this](PromptFogEnum fog) { updateFog(fog); });
+    observer.sig2_fogChanged.connect(m_lifetime,
+                                     [this](const PromptFogEnum fog) { updateFog(fog); });
     observer.sig2_tick.connect(m_lifetime,
                                [this](const MumeMoment &moment) { updateCountdown(moment); });
 

src/display/Connections.cpp

@@ -246,19 +246,6 @@ void ConnectionDrawer::drawRoomConnectionsAndDoors(const RoomHandle &room)
         if (sourceWithinBounds) {
             for (const RoomId outTargetId : sourceExit.getOutgoingSet()) {
                 const auto &targetRoom = map.getRoomHandle(outTargetId);
-                if (!targetRoom) {
-                    /* DEAD CODE */
-                    qWarning() << "Source room" << sourceId.asUint32() << "("
-                               << room.getName().toQString()
-                               << ") dir=" << mmqt::toQStringUtf8(to_string_view(sourceDir))
-                               << "has target room with internal identifier"
-                               << outTargetId.asUint32() << "which does not exist!";
-                    // This would cause a segfault in the old map scheme, but maps are now rigorously
-                    // validated, so it should be impossible to have an exit to a room that does
-                    // not exist.
-                    assert(false);
-                    continue;
-                }
                 const auto &target_coord = targetRoom.getPosition();
                 const bool targetOutsideBounds = !m_bounds.contains(target_coord);
 
@@ -300,19 +287,6 @@ void ConnectionDrawer::drawRoomConnectionsAndDoors(const RoomHandle &room)
         // incoming connections
         for (const RoomId inTargetId : sourceExit.getIncomingSet()) {
             const auto &targetRoom = map.getRoomHandle(inTargetId);
-            if (!targetRoom) {
-                /* DEAD CODE */
-                qWarning() << "Source room" << sourceId.asUint32() << "("
-                           << room.getName().toQString() << ") fromdir="
-                           << mmqt::toQStringUtf8(to_string_view(opposite(sourceDir)))
-                           << " has target room with internal identifier" << inTargetId.asUint32()
-                           << "which does not exist!";
-                // This would cause a segfault in the old map scheme, but maps are now rigorously
-                // validated, so it should be impossible to have an exit to a room that does
-                // not exist.
-                assert(false);
-                continue;
-            }
 
             // Only draw the connection if the target room is within the bounds
             const Coordinate target_coord = targetRoom.getPosition();

src/global/MakeQPointer.h

@@ -22,4 +22,12 @@ NODISCARD auto makeQPointer(Args &&...args)
     // transfer ownership, now that we've confirmed that the parent object owns it
     return QPointer<T>{ptr.release()};
 }
+
+template<typename T, typename... Args>
+NODISCARD QScopedPointer<T> makeQScopedPointer(Args &&...args)
+{
+    static_assert(std::is_base_of_v<QObject, T>);
+    auto ptr = std::make_unique<T>(std::forward<Args>(args)...);
+    return QScopedPointer<T>{ptr.release()};
+}
 } // namespace mmqt

src/global/logging.h

@@ -54,11 +54,11 @@ struct NODISCARD AbstractDebugOStream
 public:
     AbstractDebugOStream &operator<<(const char *const s)
     {
-        assert(s != nullptr);
-        auto &self = *this;
-        if (s != nullptr) {
-            self.writeUtf8(s);
+        if (s == nullptr) {
+            throw NullPointerException();
         }
+        auto &self = *this;
+        self.writeUtf8(s);
         return self;
     }
 

src/global/progresscounter.h

@@ -37,7 +37,7 @@ class NODISCARD ProgressCounter final
             if (expected == 0) {
                 return 0;
             }
-            return std::clamp<size_t>((100 * seen) / expected, 0, 99);
+            return std::clamp<size_t>((100 * seen) / expected, 0, 100);
         }
         void reset(size_t expected_ = 0)
         {

src/global/utils.cpp

@@ -14,6 +14,45 @@
 #include <QByteArray>
 #include <qglobal.h>
 
+namespace utils::details {
+
+static_assert(!cpp17::IsValidCircularSize_v<0, bool>);
+static_assert(!cpp17::IsValidCircularSize_v<1, bool>);
+static_assert(cpp17::IsValidCircularSize_v<2, bool>);
+static_assert(!cpp17::IsValidCircularSize_v<3, bool>);
+
+static_assert(!cpp17::IsValidCircularSize_v<0, uint8_t>);
+static_assert(!cpp17::IsValidCircularSize_v<1, uint8_t>);
+static_assert(cpp17::IsValidCircularSize_v<2, uint8_t>);
+static_assert(cpp17::IsValidCircularSize_v<3, uint8_t>);
+static_assert(cpp17::IsValidCircularSize_v<255, uint8_t>);
+static_assert(cpp17::IsValidCircularSize_v<256, uint8_t>);
+static_assert(!cpp17::IsValidCircularSize_v<257, uint8_t>);
+
+static_assert(cpp17::IsValidCircularSize_v<65536, uint16_t>);
+static_assert(!cpp17::IsValidCircularSize_v<65537, uint16_t>);
+
+static_assert(circular_increment<2, bool>(false) == true);
+static_assert(circular_increment<2, bool>(true) == false);
+static_assert(circular_decrement<2, bool>(false) == true);
+static_assert(circular_decrement<2, bool>(true) == false);
+
+static_assert(circular_increment<255, uint8_t>(254) == 0);
+static_assert(circular_increment<256, uint8_t>(255) == 0);
+
+static_assert(circular_increment<3, uint32_t>(0) == 1);
+static_assert(circular_increment<3, uint32_t>(1) == 2);
+static_assert(circular_increment<3, uint32_t>(2) == 0);
+static_assert(circular_increment<3, uint32_t>(3) == 1); // doesn't throw; just wraps.
+
+static_assert(circular_decrement<3, uint32_t>(0) == 2);
+static_assert(circular_decrement<3, uint32_t>(1) == 0);
+static_assert(circular_decrement<3, uint32_t>(2) == 1);
+static_assert(circular_decrement<3, uint32_t>(3) == 2); // doesn't throw; just wraps.
+static_assert(circular_decrement<3, uint32_t>(4) == 0); // doesn't throw; just wraps.
+
+} // namespace utils::details
+
 namespace utils::details::tests {
 
 static_assert(!isBitMask<int32_t>());

src/global/utils.h

@@ -167,6 +167,55 @@ NODISCARD bool isSet(const T src, const T bit)
     return (src & bit) != T{};
 }
 
+namespace details {
+namespace cpp11 {
+template<size_t N, typename T>
+struct IsValidCircularSize
+    : std::bool_constant<(N > 1 and std::is_unsigned_v<T>
+                          and (N <= std::numeric_limits<T>::max()
+                               or (sizeof(T) < sizeof(size_t)
+                                   and N == static_cast<size_t>(std::numeric_limits<T>::max()) + 1)))>
+      //
+      {};
+
+template<>
+struct IsValidCircularSize<2, bool> : std::true_type
+{};
+
+} // namespace cpp11
+
+namespace cpp17 {
+template<size_t N, typename T>
+inline constexpr bool IsValidCircularSize_v = cpp11::IsValidCircularSize<N, T>::value;
+
+} // namespace cpp17
+} // namespace details
+
+template<size_t N, typename T>
+NODISCARD constexpr T circular_increment(const T x)
+{
+    if constexpr (std::is_unsigned_v<T> && details::cpp17::IsValidCircularSize_v<N, T>) {
+        return static_cast<T>((x + 1) % N);
+    } else if constexpr (std::is_same_v<T, bool> && N == 2) {
+        return !x;
+    } else {
+        static_assert(std::is_same_v<T, void>);
+        std::abort();
+    }
+}
+template<size_t N, typename T>
+NODISCARD constexpr T circular_decrement(const T x)
+{
+    if constexpr (std::is_unsigned_v<T> && details::cpp17::IsValidCircularSize_v<N, T>) {
+        return static_cast<T>((x + N - 1) % N);
+    } else if constexpr (std::is_same_v<T, bool> && N == 2) {
+        return !x;
+    } else {
+        static_assert(std::is_same_v<T, void>);
+        std::abort();
+    }
+}
+
 } // namespace utils
 
 template<typename T>