Skip to main content
Syntax error
Source Link
Toby Speight
  • 88.3k
  • 14
  • 104
  • 327

I'd be inclined to write that big lambda as a private static function:

class interval {
private:
    static void callback(void* this_interval) {
        reinterpret_cast<interval*>(this_interval)->do_it();
    }

    void do_it() {
        if (running && function) {
            function(); 
        }
        // Check again as the called function might stop the interval
        if (running) {
            start();
        }
    }
};

Note that I've added a check that function isn't empty - particularly important given that's the state of a default-constructed interval. You may prefer to just let it throw std::bad_function_call - if so, that's certainly worth a comment.


I'd re-order the condition in start() so it doesn't need to recurse:

void interval::start() {
    if (running) { stop(); }

    running = true;
    // etc.

I see there's a reasonable move-assignment operator, but what about move construction? That needs to be implemented or explicitly deleted. And copy construct or assignment? If you explicitly delete or default copy/move assignment and constructor, it helps show which operations you've considered.


Also, be aware that if function() takes some time to run, then starting the timer after it has finished will gradually drift from a standard repeating timer. That may or may not be a concern for your use, but make sure you've thought about it!

I'd be inclined to write that big lambda as a private static function:

class interval {
private:
    static void(void* this_interval) {
        reinterpret_cast<interval*>(this_interval)->do_it();
    }

    void do_it() {
        if (running && function) {
            function(); 
        }
        // Check again as the called function might stop the interval
        if (running) {
            start();
        }
    }
};

Note that I've added a check that function isn't empty - particularly important given that's the state of a default-constructed interval. You may prefer to just let it throw std::bad_function_call - if so, that's certainly worth a comment.


I'd re-order the condition in start() so it doesn't need to recurse:

void interval::start() {
    if (running) { stop(); }

    running = true;
    // etc.

I see there's a reasonable move-assignment operator, but what about move construction? That needs to be implemented or explicitly deleted. And copy construct or assignment? If you explicitly delete or default copy/move assignment and constructor, it helps show which operations you've considered.


Also, be aware that if function() takes some time to run, then starting the timer after it has finished will gradually drift from a standard repeating timer. That may or may not be a concern for your use, but make sure you've thought about it!

I'd be inclined to write that big lambda as a private static function:

class interval {
private:
    static void callback(void* this_interval) {
        reinterpret_cast<interval*>(this_interval)->do_it();
    }

    void do_it() {
        if (running && function) {
            function(); 
        }
        // Check again as the called function might stop the interval
        if (running) {
            start();
        }
    }
};

Note that I've added a check that function isn't empty - particularly important given that's the state of a default-constructed interval. You may prefer to just let it throw std::bad_function_call - if so, that's certainly worth a comment.


I'd re-order the condition in start() so it doesn't need to recurse:

void interval::start() {
    if (running) { stop(); }

    running = true;
    // etc.

I see there's a reasonable move-assignment operator, but what about move construction? That needs to be implemented or explicitly deleted. And copy construct or assignment? If you explicitly delete or default copy/move assignment and constructor, it helps show which operations you've considered.


Also, be aware that if function() takes some time to run, then starting the timer after it has finished will gradually drift from a standard repeating timer. That may or may not be a concern for your use, but make sure you've thought about it!

Source Link
Toby Speight
  • 88.3k
  • 14
  • 104
  • 327

I'd be inclined to write that big lambda as a private static function:

class interval {
private:
    static void(void* this_interval) {
        reinterpret_cast<interval*>(this_interval)->do_it();
    }

    void do_it() {
        if (running && function) {
            function(); 
        }
        // Check again as the called function might stop the interval
        if (running) {
            start();
        }
    }
};

Note that I've added a check that function isn't empty - particularly important given that's the state of a default-constructed interval. You may prefer to just let it throw std::bad_function_call - if so, that's certainly worth a comment.


I'd re-order the condition in start() so it doesn't need to recurse:

void interval::start() {
    if (running) { stop(); }

    running = true;
    // etc.

I see there's a reasonable move-assignment operator, but what about move construction? That needs to be implemented or explicitly deleted. And copy construct or assignment? If you explicitly delete or default copy/move assignment and constructor, it helps show which operations you've considered.


Also, be aware that if function() takes some time to run, then starting the timer after it has finished will gradually drift from a standard repeating timer. That may or may not be a concern for your use, but make sure you've thought about it!