Refactor SGTimerQueue

Use std::unique_ptrs to handle SGTimer
Use std::vector as container together with STL heap functions

simgear/structure/event_mgr.cxx

@@ -4,8 +4,21 @@
 
 #include "event_mgr.hxx"
 
+#include <algorithm>
+
 #include <simgear/debug/logstream.hxx>
 
+SGTimer::~SGTimer()
+{
+    delete callback;
+    callback = nullptr;
+}
+
+void SGTimer::run()
+{
+    (*callback)();
+}
+
 void SGEventMgr::add(const std::string& name, SGCallback* cb,
                      double interval, double delay,
                      bool repeat, bool simtime)
@@ -20,27 +33,16 @@ void SGEventMgr::add(const std::string& name, SGCallback* cb,
     if(delay <= 0) delay = 1e-6;
     if(interval <= 0) interval = 1e-6; // No timer endless loops please...
 
-    SGTimer* t = new SGTimer;
+    auto t = std::make_unique<SGTimer>();
     t->interval = interval;
     t->callback = cb;
     t->repeat = repeat;
     t->name = name;
     t->running = false;
     
-    SGTimerQueue* q = simtime ? &_simQueue : &_rtQueue;
+    SGTimerQueue& q = simtime ? _simQueue : _rtQueue;
 
-    q->insert(t, delay);
+    q.insert(std::move(t), delay);
-}
-
-SGTimer::~SGTimer()
-{
-    delete callback;
-    callback = NULL;
-}
-
-void SGTimer::run()
-{
-    (*callback)();
 }
 
 SGEventMgr::SGEventMgr() :
@@ -100,22 +102,18 @@ void SGEventMgr::removeTask(const std::string& name)
         return;
     }
 
-  SGTimer* t = _simQueue.findByName(name);
+    bool removedSim = false;
-  if (t) {
+    bool removeRT = false;
-    _simQueue.remove(t);
+
-  } else if ((t = _rtQueue.findByName(name))) {
+    removedSim = _simQueue.removeByName(name);
-    _rtQueue.remove(t);
+    if(!removedSim) {
-  } else {
+        removeRT = _rtQueue.removeByName(name);
-    SG_LOG(SG_GENERAL, SG_WARN, "removeTask: no task found with name:" << name);
+    }
-    return;
+
-  }
+    if(!removedSim && !removeRT) {
-  if (t->running) {
+        SG_LOG(SG_GENERAL, SG_WARN, "removeTask: no task found with name:" << name);
-    // mark as not repeating so that the SGTimerQueue::update()
+        return;
-    // will clean it up
+    }
-    t->repeat = false;
-  } else {
-    delete t;
-  }
 }
 
 void SGEventMgr::dump()
@@ -133,164 +131,84 @@ SGSubsystemMgr::Registrant<SGEventMgr> registrantSGEventMgr(
 
 ////////////////////////////////////////////////////////////////////////
 // SGTimerQueue
-// This is the priority queue implementation:
 ////////////////////////////////////////////////////////////////////////
 
-SGTimerQueue::SGTimerQueue(int size)
-{
-    _now = 0;
-    _numEntries = 0;
-    _tableSize = 1;
-    while(size > _tableSize)
-        _tableSize = ((_tableSize + 1)<<1) - 1;
-
-    _table = new HeapEntry[_tableSize];
-    for(int i=0; i<_tableSize; i++) {
-	    _table[i].pri = 0;
-        _table[i].timer = 0;
-    }
-}
-
-
-SGTimerQueue::~SGTimerQueue()
-{
-    clear();
-    delete[] _table;
-}
-
 void SGTimerQueue::clear()
 {
-    // delete entries
+    _table.clear();
-    for(int i=0; i<_numEntries; i++) {
-        delete _table[i].timer;
-    }
-    
-    _numEntries = 0;
-    
-    // clear entire table to empty
-    for(int i=0; i<_tableSize; i++) {
-	    _table[i].pri = 0;
-        _table[i].timer = 0;
-    }
 }
-int maxTimerQueuePerItem_us = 30;
+
 void SGTimerQueue::update(double deltaSecs, std::map<std::string, double> &timingStats)
 {
     _now += deltaSecs;
 
-    while (_numEntries && nextTime() <= _now) {
+    while (!_table.empty() && nextTime() <= _now) {
-        SGTimer* t = remove();
+        _current_timer = remove();
-        if (t->repeat)
+
-            insert(t, t->interval);
         // warning: this is not thread safe
         // but the entire timer queue isn't either
         SGTimeStamp timeStamp;
         timeStamp.stamp();
-        t->running = true;
+        _current_timer->running = true;
-        t->run();
+        _current_timer->run();
-        t->running = false;
+        _current_timer->running = false;
-        timingStats[t->name] += timeStamp.elapsedMSec() / 1000.0;
+        timingStats[_current_timer->name] += timeStamp.elapsedMSec() / 1000.0;
-        if (!t->repeat)
+
-            delete t;
+        // insert() after run() because the timer can remove itself with removeByName()
+        if(_current_timer->repeat) {
+            double interval = _current_timer->interval;
+            insert(std::move(_current_timer), interval);
+        }
+
+        _current_timer = nullptr;
+
     }
 }
 
-void SGTimerQueue::insert(SGTimer* timer, double time)
+std::unique_ptr<SGTimer> SGTimerQueue::remove()
 {
-    if(_numEntries >= _tableSize)
+    if(_table.empty()) {
-	growArray();
+        return nullptr;
-
-    _numEntries++;
-    _table[_numEntries-1].pri = -(_now + time);
-    _table[_numEntries-1].timer = timer;
-
-    siftUp(_numEntries-1);
-}
-
-SGTimer* SGTimerQueue::remove(SGTimer* t)
-{
-    int entry;
-    for(entry=0; entry<_numEntries; entry++)
-        if(_table[entry].timer == t)
-            break;
-    if(entry == _numEntries)
-        return 0;
-
-    // Swap in the last item in the table, and sift down
-    swap(entry, _numEntries-1);
-    _numEntries--;
-    siftDown(entry);
-
-    return t;
-}
-
-SGTimer* SGTimerQueue::remove()
-{
-    if(_numEntries == 0) {
-	return 0;
-    } else if(_numEntries == 1) {
-	_numEntries = 0;
-	return _table[0].timer;
     }
 
-    SGTimer *result = _table[0].timer;
+    std::pop_heap(_table.begin(), _table.end(), std::greater<>());
-    _table[0] = _table[_numEntries - 1];
+    auto ptr = std::move(_table[_table.size()-1].timer);
-    _numEntries--;
+    _table.pop_back();
-    siftDown(0);
+    return ptr;
-    return result;
 }
 
-void SGTimerQueue::siftDown(int n)
-{
-    // While we have children bigger than us, swap us with the biggest
-    // child.
-    while(lchild(n) < _numEntries) {
-        int bigc = lchild(n);
-        if(rchild(n) < _numEntries && pri(rchild(n)) > pri(bigc))
-            bigc = rchild(n);
-        if(pri(bigc) <= pri(n))
-            break;
-        swap(n, bigc);
-        n = bigc;
-    }
-}
 
-void SGTimerQueue::siftUp(int n)
+void SGTimerQueue::insert(std::unique_ptr<SGTimer> timer, double time)
 {
-    while((n != 0) && (_table[n].pri > _table[parent(n)].pri)) {
+    _table.push_back({_now + time, std::move(timer)});
-	swap(n, parent(n));
+    std::push_heap(_table.begin(), _table.end(), std::greater<>());
-	n = parent(n);
-    }
-    siftDown(n);
-}
-
-void SGTimerQueue::growArray()
-{
-    _tableSize = ((_tableSize+1)<<1) - 1;
-    HeapEntry *newTable = new HeapEntry[_tableSize];
-    for(int i=0; i<_numEntries; i++) {
-	newTable[i].pri  = _table[i].pri;
-	newTable[i].timer = _table[i].timer;
-    }
-    delete[] _table;
-    _table = newTable;
-}
-
-SGTimer* SGTimerQueue::findByName(const std::string& name) const
-{
-  for (int i=0; i < _numEntries; ++i) {
-    if (_table[i].timer->name == name) {
-      return _table[i].timer;
-    }
-  }
-  
-  return NULL;
 }
 
 void SGTimerQueue::dump()
 {
-    for (int i=0; i < _numEntries; ++i) {
+    for (const Entry &entry : _table) {
-        const auto t = _table[i].timer;
+        const auto &t = entry.timer;
         SG_LOG(SG_GENERAL, SG_INFO, "\ttimer:" << t->name << ", interval=" << t->interval);
     }
 }
+
+bool SGTimerQueue::removeByName(const string &name) {
+    for (size_t i=0; i < _table.size(); ++i) {
+        if (_table[i].timer->name == name) {
+            std::pop_heap(_table.begin()+i, _table.end(), std::greater<>());
+            _table.pop_back();
+            if(i != 0) {
+              std::make_heap(_table.begin(), _table.end(), std::greater<>());
+            }
+            return true;
+        }
+    }
+
+    // Not found in queue, but maybe the timer is currently running
+    if(_current_timer && _current_timer->name == name) {
+        _current_timer->repeat = false;
+        return true;
+    }
+
+    return false;
+}

simgear/structure/event_mgr.hxx

@@ -11,63 +11,51 @@ class SGEventMgr;
 class SGTimer
 {
 public:
+    SGTimer() = default;
     ~SGTimer();
     void run();
 
     std::string name;
-    double interval;
+    double interval = 0.0;
-    SGCallback* callback;
+    SGCallback* callback = nullptr;
-    bool repeat;
+    bool repeat = false;
-    bool running;
+    bool running = false;
+
+    // Allow move only
+    SGTimer& operator=(const SGTimer &) = delete;
+    SGTimer(const SGTimer &other) = delete;
+    SGTimer& operator=(SGTimer &&) = default;
+    SGTimer(SGTimer &&other) = default;
 };
 
+/*! Queue to execute SGTimers after given delays */
 class SGTimerQueue
 {
 public:
-    SGTimerQueue(int preSize=1);
+    SGTimerQueue() = default;
-    ~SGTimerQueue();
+    ~SGTimerQueue() = default;
+
     void clear();
     void update(double deltaSecs, std::map<std::string, double> &timingStats);
-
+    void insert(std::unique_ptr<SGTimer> timer, double time);
-    double now() { return _now; }
+    bool removeByName(const std::string& name);
-
-    void     insert(SGTimer* timer, double time);
-    SGTimer* remove(SGTimer* timer);
-    SGTimer* remove();
-
-    SGTimer* nextTimer() { return _numEntries ? _table[0].timer : 0; }
-    double   nextTime()  { return -_table[0].pri; }
-
-    SGTimer* findByName(const std::string& name) const;
 
     void dump();
 
 private:
-    // The "priority" is stored as a negative time.  This allows the
+    std::unique_ptr<SGTimer> remove();
-    // implementation to treat the "top" of the heap as the largest
+    double nextTime() const { return _table[0].pri; }
-    // value and avoids developer mindbugs. ;)
-    struct HeapEntry { double pri; SGTimer* timer; };
 
-    int parent(int n) { return ((n+1)/2) - 1; }
+    struct Entry {
-    int lchild(int n) { return ((n+1)*2) - 1; }
+        double pri;
-    int rchild(int n) { return ((n+1)*2 + 1) - 1; }
+        std::unique_ptr<SGTimer> timer;
-    double pri(int n) { return _table[n].pri; }
-    void swap(int a, int b) {
-        HeapEntry tmp = _table[a];
-        _table[a] = _table[b];
-        _table[b] = tmp;
-    }
-    void siftDown(int n);
-    void siftUp(int n);
-    void growArray();
 
-    // gcc complains there is no function specification anywhere.
+        bool operator>(const Entry& other) const { return pri > other.pri; }
-    // void check();
+    };
 
-    double _now;
+    std::unique_ptr<SGTimer> _current_timer;
-    HeapEntry *_table;
+    double _now = 0.0;
-    int _numEntries;
+    std::vector<Entry> _table;
-    int _tableSize;
 };
 
 class SGEventMgr : public SGSubsystem