1
\$\begingroup\$

The purpose of this piece of code is to organize function calls in to different time frames. Each frame can be identified by a major frame number and a minor frame number:

enter image description here

Code

My strategy has been to develop a task manager that allows to register functions as follows:

FrameTaskManager.h

#include <array>
#include <map>
#include <vector>
#include <functional>

class FrameTaskManager {
public:
    using task_t = std::function<void()>;
    static const int frameSize = 16;

    struct Frame {
        Frame() = default;
        Frame(int major, int minor) {
            this->major = major;
            this->minor = minor;
        }
        int major = 0;
        int minor = 0;
        Frame operator+(int increment) {
            Frame tmp(*this);
            tmp.major = (tmp.major + (tmp.minor + increment) / frameSize);
            tmp.minor = (tmp.minor + increment) % frameSize;
            return tmp;
        }
    };
    void step();
    Frame currentFrame() { return _frameCounter.frame; }
    void addTaskAtFrame(Frame frame, task_t task);
    void addRecurrentTask(int minor, task_t task);

private:
    struct FrameCounter {
        Frame frame;
        void next() { frame = frame + 1; }
    } _frameCounter;
    std::array<std::vector<task_t>, frameSize> _recurrenTasks;
    std::map<int, std::map<int, std::vector<task_t>>>_tasks;
}

FrameTaskManager.cpp

#include "frameTaskManager.hpp"

using task_t = std::function<void()>;

void FrameTaskManager::step() {
    const int minor = _frameCounter.frame.minor;
    const int major = _frameCounter.frame.major;
    // Run recurrent tasks
    for (auto& task : _recurrenTasks[minor]) {
        task();
    }
    // Run and pop scheduled tasks
    while (!_tasks[major][minor].empty()){
        auto task = _tasks[major][minor].back();
        task();
        _tasks[major][minor].pop_back();
    }
    _frameCounter.next();
}

void FrameTaskManager::addTaskAtFrame(Frame frame, task_t task) {
    _tasks[frame.major][frame.minor].push_back(task);
}

void FrameTaskManager::addRecurrentTask(int minor, task_t task) {
    _recurrenTasks[minor].push_back(task);
}

Usage example main.cpp

#include <iostream>

void task1(){
    std::cout << "task1!\n";
};
void task2(int a){
    std::cout << "task2 says: " << a << "\n";
};
void task3() {
    std::cout << "task3!\n";
};

int main() {
    FrameTaskManager frameTaskManager;
    frameTaskManager.addRecurrentTask(2,task1);
    frameTaskManager.addRecurrentTask(4,[a=int(4)](){task2(a);});
    frameTaskManager.addTaskAtFrame(FrameTaskManager::Frame(1,1), task3);

    for (int step=0; step<30; ++step) {
        std::cout << "[" << frameTaskManager.currentFrame().major 
                  << "," << frameTaskManager.currentFrame().minor << "]\n";

        frameTaskManager.step();
    }
}

output:

[0,0]
[0,1]
[0,2]
task1!
[0,3]
[0,4]
task2 says: 4
[0,5]
[0,6]
[0,7]
[0,8]
[0,9]
[0,10]
[0,11]
[0,12]
[0,13]
[0,14]
[0,15]
[1,0]
[1,1]
task3!
[1,2]
task1!
[1,3]
[1,4]
task2 says: 4
[1,5]
[1,6]
[1,7]
[1,8]
[1,9]
[1,10]
[1,11]
[1,12]
[1,13]

Questions and advice

This is my first time playing around with std::function any advices or ideas? What do you like about the implementation? What you don't like? Any advices on the style? Thank you very much for your reviews!

I also don't fully understand why I cannot do the following in the FrameTaskManager.h:

Frame(int major, int minor) : major(major), minor(minor) {}

When I do that I get the following compiling error:

prog.cc: In constructor 'FrameTaskManager::Frame::Frame(int, int)':
prog.cc:13:37: error: class 'FrameTaskManager::Frame' does not have any field named 'gnu_dev_major'
       Frame(int major, int minor) : major(major), minor(minor) {}
                                     ^~~~~
prog.cc:13:51: error: class 'FrameTaskManager::Frame' does not have any field named 'gnu_dev_minor'
       Frame(int major, int minor) : major(major), minor(minor) {}
                                                   ^~~~~

What do you think about implementing small functions in the header file? Is this a bad practice? I think they might help to explain the code, not sure if I should move all the implementations to the cpp or if it is fine that I've implemented some of them in the header.

\$\endgroup\$
7
  • \$\begingroup\$ What you you mean by "I cannot do the following" in the final sentence? Do you get an error? I don't see why you should. Or does your local style guide prohibit you? Again, I don't see why it should. \$\endgroup\$ Feb 28, 2018 at 8:14
  • \$\begingroup\$ I think he gets wrong results due to aliasing. major(major) might initialize major with its previous (random) value \$\endgroup\$
    – miscco
    Feb 28, 2018 at 9:11
  • \$\begingroup\$ @TobySpeight Oh, sorry. I forgot to explain it. It doesn't compile if I do that. I've added the error \$\endgroup\$
    – Blasco
    Feb 28, 2018 at 9:48
  • 2
    \$\begingroup\$ It looks like you have preprocessor macros called minor() and major(), probably from one of the included headers. Arguably a bug in the compiler/library, given you only include standard headers. I think you won't hit that if you use brace initialization instead: minor{minor}, major{major} (since you've had no answers yet, you might be able to jump in and fix it). \$\endgroup\$ Feb 28, 2018 at 9:59
  • 2
    \$\begingroup\$ @miscco: no, it's perfectly correct to use a parameter of the same name as the field you're initialising. \$\endgroup\$ Feb 28, 2018 at 10:01

1 Answer 1

1
\$\begingroup\$

misc

  • FrameTaskManager::currentFrame() should be const.

  • There's no need for using task_t = std::function<void()>; in FrameTaskManager.cpp.


frame class

  • I don't think there's a lot of benefit to providing a default-constructor and default values for Frame. It allows the user to not think about what the values should be... which isn't actually a good thing. (And in this case it enables a bug, detailed below).

  • Frame(int major, int minor) Could use some error checking (assert / throw) to ensure minor frame is valid.

  • major and minor should probably be unsigned types, since (minor at least) can never be negative. minor could also use a smaller integral type if desired. This does require more thought when doing arithmetic, but that's not necessarily a bad thing...

  • Either way, these types should have using declarations or typedefs in the FrameManager class, which can be used for passing arguments etc.

  • (Bug:) auto f = FrameTaskManager::Frame(0, 0) + (-5) gives a frame of (0, -5), which probably isn't desired behaviour. Using unsigned types as suggested above makes it more obvious that we have to think about / check this stuff from the start (even if it doesn't make the actual arithmetic any easier).

  • operator+ The current operator+ seems like a bit of a special-case compromise. The pre-increment operator (or better, a function called advance()) would provide the needed functionality. An advance(int major, int minor) version could also be added to handle changing both values if necessary.

  • The FrameCounter class is not needed - we just need a Frame _currentFrame member.


task scheduling

  • addTaskAtFrame() and addRecurrentTask() could use some error checking to ensure we don't add a task at a previous frame.

  • (BUG:) The std::map::operator[] actually adds an element to the map if there is no entry with that key. This makes the current code to run scheduled tasks Very Bad, because the size of _tasks and / or its inner map will increase with every call to step()!

  • It would be much cleaner to use a std::multimap<Frame, task_t>. With an appropriate operator< for Frame.

  • (One other thing to be aware of with containers of functions like this: one scheduled task may attempt to add another scheduled task to the frame manager. This would cause problems by altering the _tasks or _recurrentTasks members while we're iterating through them in step().

    We could guard against this at run-time by adding a boolean member that is set to true for the duration of the step function, and asserting against it in addTaskAtFrame() and addRecurrentTask(). A more foolproof solution would be to copy / move the current frame's tasks to a temporary container before executing them, so we can freely alter the member containers themselves.

    This is all left as an exercise to the reader ;) )


modified code

#include <array>
#include <map>
#include <vector>
#include <functional>
#include <cassert>
#include <utility>

class FrameTaskManager {
public:
    using task_t = std::function<void()>;

    using frame_major_t = std::uint32_t;
    using frame_minor_t = std::uint8_t;

    static const auto frameSize = frame_minor_t{ 16 };

    struct Frame {
        Frame(frame_major_t major, frame_minor_t minor): 
            major(major), minor(minor) {
            assert(minor < frameSize);
        }

        int major;
        int minor;

        void advance() {
            assert(minor < frameSize);
            ++minor;
            if (minor == frameSize) {
                ++major;
                minor = frame_minor_t{ 0 };
            }
        }

        friend bool operator<(Frame const& f1, Frame const& f2) {
            return std::tie(f1.major, f1.minor) < std::tie(f2.major, f2.minor);
        }
        friend bool operator<=(Frame const& f1, Frame const& f2) {
            return std::tie(f1.major, f1.minor) <= std::tie(f2.major, f2.minor);
        }
    };
    void step();
    Frame currentFrame() const { return _frame; }
    void addTaskAtFrame(Frame frame, task_t task);
    void addRecurrentTask(frame_minor_t minor, task_t task);

private:
    Frame _frame = Frame{ 0, 0 };
    std::array<std::vector<task_t>, frameSize> _recurrenTasks;
    std::multimap<Frame, task_t> _tasks;
};

void FrameTaskManager::step() {
    // Run recurrent tasks
    for (auto& task : _recurrenTasks[_frame.minor]) {
        task();
    }

    // Run and pop scheduled tasks
    auto range = _tasks.equal_range(_frame);
    for (auto i = range.first; i != range.second; ++i) {
        i->second();
    }
    _tasks.erase(range.first, range.second);

    _frame.advance();
}

void FrameTaskManager::addTaskAtFrame(Frame frame, task_t task) {
    assert(_frame <= frame);
    _tasks.emplace(frame, std::move(task));
}

void FrameTaskManager::addRecurrentTask(frame_minor_t minor, task_t task) {
    assert(minor < frameSize);
    _recurrenTasks[minor].push_back(std::move(task));
}

small functions in headers

(Opinion)

I think it's best to make the header as clean as possible (i.e. declarations only) and put as much as possible in the cpp file. Class definitions in headers should be a readable summary of code / interfaces. If function definitions are needed in the header (templates, inline functions), I put them outside of the class definition, to keep it minimal.

However, I do use an IDE add-on that can add function definitions straight in the cpp file, and has shortcut keys that allow skipping around between files, and directly between function definitions and declarations. Without that, there'd be a lot more typing overhead to do

\$\endgroup\$
1
  • \$\begingroup\$ Very inspiring review! I really like the part with the std::tie Thank you very much! \$\endgroup\$
    – Blasco
    Feb 28, 2018 at 14:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.