patterncppMinor
Follow-up: Timer utilizing std::future
Viewed 0 times
stdutilizingfuturetimerfollow
Problem
The previous code (as displayed in Timer utilizing std::future) is now:
```
#include
#include
#include
#include
#include
#include
#include
// PeriodicFunction
// =============================================================================
namespace Detail {
/// A functor to invoke a function periodically.
template
class PeriodicFunction
{
// Types
// =====
public:
typedef std::chrono::milliseconds period_type;
typedef Value value_type;
protected:
typedef std::function function_type;
typedef std::future future_type;
// Construction
// ============
protected:
/// Initialize with a function where the arguments are bounded to that function.
/// \SEE std::bind
template
PeriodicFunction(Callable&& callable, Arguments&&... arguments)
: m_function(std::bind(
std::move(callable),
std::forward(arguments)...)),
m_period(period_type::zero())
{}
public:
PeriodicFunction(const PeriodicFunction&) = delete;
PeriodicFunction& operator = (const PeriodicFunction&) = delete;
/// Calls stop.
~PeriodicFunction() { stop(); }
// Start/Stop
// ==========
/// True, if an invocation thread is active.
bool active() const noexcept { return m_thread.joinable(); }
/// Start an invocation thread and repeatedly invoke the function (in the given period)
/// after the given delay.
/// - If a previous invocation thread is active, no invocation of the function
/// takes place.
/// - After the first invocation of the function (at least one is done):
/// - If the period is or has become zero (due to a call to stop) the
/// invocation thread stops without any further invocation of the function.
/// - As long as an invocation of the function has not finished, the next
/// possible invocation is delayed by the given period.
/// \EXCEPTION All exceptions stop the invocation thread and the exceptio
```
#include
#include
#include
#include
#include
#include
#include
// PeriodicFunction
// =============================================================================
namespace Detail {
/// A functor to invoke a function periodically.
template
class PeriodicFunction
{
// Types
// =====
public:
typedef std::chrono::milliseconds period_type;
typedef Value value_type;
protected:
typedef std::function function_type;
typedef std::future future_type;
// Construction
// ============
protected:
/// Initialize with a function where the arguments are bounded to that function.
/// \SEE std::bind
template
PeriodicFunction(Callable&& callable, Arguments&&... arguments)
: m_function(std::bind(
std::move(callable),
std::forward(arguments)...)),
m_period(period_type::zero())
{}
public:
PeriodicFunction(const PeriodicFunction&) = delete;
PeriodicFunction& operator = (const PeriodicFunction&) = delete;
/// Calls stop.
~PeriodicFunction() { stop(); }
// Start/Stop
// ==========
/// True, if an invocation thread is active.
bool active() const noexcept { return m_thread.joinable(); }
/// Start an invocation thread and repeatedly invoke the function (in the given period)
/// after the given delay.
/// - If a previous invocation thread is active, no invocation of the function
/// takes place.
/// - After the first invocation of the function (at least one is done):
/// - If the period is or has become zero (due to a call to stop) the
/// invocation thread stops without any further invocation of the function.
/// - As long as an invocation of the function has not finished, the next
/// possible invocation is delayed by the given period.
/// \EXCEPTION All exceptions stop the invocation thread and the exceptio
Solution
Generally speaking, the code already seems really good. Therefore, I have nothing more than a few notes:
-
It is more of a matter of taste, but I would use some type aliases instead of the old
-
C++ has special syntax to import type names from a base class without having to use a full
-
-
In their answer to your previous question, @ruds said that
-
Really no more than a tidbit, but namespaces are generally lowercase. Consider replacing
-
Functions that return a
Unfortunately, the standard library containers have a function
Discussing what can be improved is good, but I also want to put a note about what you did right: tag dispatching with
-
It is more of a matter of taste, but I would use some type aliases instead of the old
typedef. The type aliases can be templated (contrary to typedef) and provide a syntax that is close to variable assignements (auto a = 89;) and namespace aliases (namespace foobar = foo::bar;). That means that you can write code which looks more consistent:using period_type = std::chrono::milliseconds;
using value_type = Value;
using function_type = std::function;
using future_type = std::future;-
C++ has special syntax to import type names from a base class without having to use a full
typedef or type alias, you can use it to reduce the amount of code and make explicit your intent in PeriodicFunction:using typename Base::period_type;
using typename Base::value_type;-
typedef std::mutex mutex; seems useless and potentially confusing. I would drop the typedef and use std::mutex everywhere instead. It shouldn't hinder readability.-
In their answer to your previous question, @ruds said that
Timer::m_results should not be mutable, which is right, but I see no harm in having the corresponding std::mutex be mutable. It does not seem to be currently useful though.-
Really no more than a tidbit, but namespaces are generally lowercase. Consider replacing
Detail by detail.-
Functions that return a
bool tend to be more understandable when their name is more than just a name. For example, bool exception() {} makes me think that the function returns an std::exception or a derived class (which would be odd). Consider renaming it exception_occured instead. Also, consider renaming active to is_active.Unfortunately, the standard library containers have a function
empty while is_empty would have been a better name ("empty" somehow means "empty that container" to me); I understand that you keep this particular name for consistency.Discussing what can be improved is good, but I also want to put a note about what you did right: tag dispatching with
std::true_type and std::false_type is great. It helps to write simple and readable code and to avoid some std::enable_if and template boilerplate. You also correctly implemented perfect forwarding and used proper scoped locks. Kudos for that :)Code Snippets
using period_type = std::chrono::milliseconds;
using value_type = Value;
using function_type = std::function<Value ()>;
using future_type = std::future<Value>;using typename Base::period_type;
using typename Base::value_type;Context
StackExchange Code Review Q#47347, answer score: 8
Revisions (0)
No revisions yet.