Made Clock destruction safe

SDL does not guard against the final callback still executing when the
call to remove the timer returns. So we have to make sure the object
we access on the callback is not prematurely destructed. This commit
uses shared_ptr and weak_ptr to ensure that.

Note that we sort-of have a singleton again, only now it is private
and safely destructed.

src/clock.cpp

@@ -3,9 +3,46 @@
 #include "debug.h"
 #include "inputmanager.h"
 
+#include <SDL.h>
 #include <sys/time.h>
 
 
+class Clock::Timer {
+public:
+	Timer();
+	~Timer();
+	void start();
+	void getTime(unsigned int &hours, unsigned int &minutes);
+	unsigned int callback();
+
+private:
+	unsigned int update();
+
+	SDL_TimerID timerID;
+	int minutes, hours;
+};
+
+static std::weak_ptr<Clock::Timer> globalTimer;
+
+/**
+ * Gets the global timer instance, or create it if it doesn't exist already.
+ * This function is not thread safe: only one thread at a time may call it.
+ */
+static std::shared_ptr<Clock::Timer> globalTimerInstance()
+{
+	std::shared_ptr<Clock::Timer> timer = globalTimer.lock();
+	if (timer) {
+		return timer;
+	} else {
+		// Note: Separate start method is necessary because globalTimer must
+		//       be written before callbacks can occur.
+		timer.reset(new Clock::Timer());
+		globalTimer = timer;
+		timer->start();
+		return timer;
+	}
+}
+
 static void notify()
 {
 	SDL_UserEvent e = {
@@ -20,47 +57,51 @@ static void notify()
 	SDL_PushEvent((SDL_Event *) &e);
 }
 
-extern "C" Uint32 clockCallbackFunc(Uint32 timeout, void *d);
-
-class Clock::Forwarder {
-	static unsigned int clockCallbackFunc(Clock *clock)
+extern "C" Uint32 callbackFunc(Uint32 /*timeout*/, void */*d*/)
 {
-		return clock->clockCallback();
-	}
-	friend Uint32 clockCallbackFunc(Uint32 timeout, void *d);
-};
-
-extern "C" Uint32 clockCallbackFunc(Uint32 timeout, void *d)
-{
-	return Clock::Forwarder::clockCallbackFunc(static_cast<Clock *>(d));
+	std::shared_ptr<Clock::Timer> timer = globalTimer.lock();
+	return timer ? timer->callback() : 0;
 }
 
-unsigned int Clock::clockCallback()
+Clock::Timer::Timer()
+	: timerID(NULL)
 {
+	tzset();
+}
+
+Clock::Timer::~Timer()
+{
+	if (timerID) {
+		SDL_RemoveTimer(timerID);
+	}
+}
+
+void Clock::Timer::start()
+{
+	if (timerID) {
+		ERROR("SDL timer was already started\n");
+		return;
+	}
 	unsigned int ms = update();
-	notify();
-	return ms;
+	timerID = SDL_AddTimer(ms, callbackFunc, this);
+	if (!timerID) {
+		ERROR("Could not initialize SDL timer: %s\n", SDL_GetError());
+	}
 }
 
-std::string Clock::getTime(bool is24)
+void Clock::Timer::getTime(unsigned int &hours, unsigned int &minutes)
 {
-	char buf[9];
-	int h = hours;
-	bool pm = hours >= 12;
-
-	if (!is24 && pm)
-		h -= 12;
-
-	sprintf(buf, "%02i:%02i%s", h, minutes, is24 ? "" : (pm ? "pm" : "am"));
-	return std::string(buf);
+	hours = this->hours;
+	minutes = this->minutes;
 }
 
-unsigned int Clock::update()
+unsigned int Clock::Timer::update()
 {
 	struct timeval tv;
 	struct tm result;
 	gettimeofday(&tv, NULL);
 	localtime_r(&tv.tv_sec, &result);
+	// TODO: Store both in a single 32-bit write?
 	minutes = result.tm_min;
 	hours = result.tm_hour;
 	DEBUG("Time updated: %02i:%02i:%02i\n", hours, minutes, result.tm_sec);
@@ -69,28 +110,41 @@ unsigned int Clock::update()
 	// We don't need high precision, but it is important that any deviation is
 	// past the minute mark, so the fetched hour and minute number belong to
 	// the freshly started minute.
+	// TODO: Does the SDL timer in fact guarantee we're never called early?
+	//       "ms = t->interval - SDL_TIMESLICE;" worries me.
 	// Clamping it at 1 sec both avoids overloading the system in case our
 	// computation goes haywire and avoids passing 0 to SDL, which would stop
 	// the recurring timer.
 	return std::max(1, (60 - result.tm_sec)) * 1000;
 }
 
-void Clock::addTimer(int timeout)
+unsigned int Clock::Timer::callback()
 {
-	timer = SDL_AddTimer(timeout, clockCallbackFunc, this);
-	if (timer == NULL)
-		ERROR("Could not initialize SDLTimer: %s\n", SDL_GetError());
+	unsigned int ms = update();
+	notify();
+	// TODO: SDL timer forgets adjusted interval if a timer was inserted or
+	//       removed during the callback. So we should either fix that bug
+	//       in SDL or ensure we don't insert/remove timers at runtime.
+	//       The blanking timer is inserted/removed quite a lot at time moment,
+	//       but it could be reprogrammed to adjust the interval instead.
+	return ms;
+}
+
+std::string Clock::getTime(bool is24)
+{
+	unsigned int hours, minutes;
+	timer->getTime(hours, minutes);
+
+	bool pm = hours >= 12;
+	if (!is24 && pm)
+		hours -= 12;
+
+	char buf[9];
+	sprintf(buf, "%02i:%02i%s", hours, minutes, is24 ? "" : (pm ? "pm" : "am"));
+	return std::string(buf);
 }
 
 Clock::Clock()
+	: timer(globalTimerInstance())
 {
-	tzset();
-
-	unsigned int ms = update();
-	addTimer(ms);
-}
-
-Clock::~Clock()
-{
-	SDL_RemoveTimer(timer);
 }

src/clock.h

@@ -1,26 +1,32 @@
 #ifndef __CLOCK_H__
 #define __CLOCK_H__
 
+#include <memory>
 #include <string>
-#include <SDL.h>
 
+
+/**
+ * A keeper of wall clock time with minute accuracy.
+ * Sends a user event on every minute boundary, to force a repaint of the
+ * clock display.
+ */
 class Clock {
 public:
-	Clock();
-	~Clock();
+	/**
+	 * Used by implementation; please ignore.
+	 */
+	class Timer;
 
+	Clock();
+
+	/**
+	 * Gets a string representation of the current time.
+	 * Uses 24-hour format if is24 is true, otherwise AM/PM.
+	 */
 	std::string getTime(bool is24 = true);
 
-	class Forwarder;
-	friend Forwarder;
-
 private:
-	void addTimer(int timeout);
-	unsigned int update();
-	unsigned int clockCallback();
-
-	SDL_TimerID timer;
-	int minutes, hours;
+	std::shared_ptr<Timer> timer;
 };
 
 #endif /* __CLOCK_H__ */