Skip to main content
Commonmark migration
Source Link

###Use aliases

Use aliases

Instead of writing std::chrono::system_clock:: everywhere, simply create an alias:

using clock_type = std::chrono::system_clock;

This is a good scenario for an alias because it shortens your typing. More importantly, you have defined what clock type you use in one specific place; this allows you to change the clock type at one spot and have it affect your entire class.

You could even make it a template parameter if you want to allow users to provide their own clock type.


###Constructor

Constructor

I'm not sure why you initialize _start inside the constructor body instead of the constructor initializer list.

CSDSTimer(std::string name, std::string context = "global",
    std::ostream &stream = std::cout)
    : _name(name)
    , _context(context)
    , _default_str(&stream)
    , _start{ clock_type::now() } // init here instead of ctor body; note alias usage
{}

Additionally, it might be cleaner if you add some new lines like I did, but in all honesty it's not that hard to read... so this one's up to you.


###Destructor

Destructor

Here, we notice that your only usage of _end is in the destructor. You don't provide any query member functions, so let's just go ahead and remove _end altogether. This will make the class smaller.

You also want milliseconds, but first take seconds and then multiply by 1000. You can directly get milliseconds by using std::chrono::duration_cast().

~CSDSTimer()
{
    // we can directly compute the elapsed time without storing `_end`
    auto elapsed_ms = std::chrono::duration_cast<std::chrono::milliseconds>(
            clock_type::now() - _start);
    
    *_default_str << "PROFILING_DATUM(\"" << _name << "\", \"" << _context
        << "\", " << elapsed_ms.count() << ", \"C++\")" // no more multiply by 1000
        << std::endl;
}

There is a small subtle bug you can avoid by doing it like shown above, but I will describe it so you can be on the look out in the future.

The bug is that the elapsed time is stored in a std::chrono::duration<double>, but _end - _start basically returns std::chrono::nanoseconds. How is this an issue? Well, std::chrono::nanoseconds must be represented by a signed integer type of at least 64 bits (see: http://en.cppreference.com/w/cpp/chrono/duration).

While a double does have 64 bits, it's certainly lacking the ability to represent values from the integer point of view. This would result in a loss of precision/erroneous results.

###Use aliases

Instead of writing std::chrono::system_clock:: everywhere, simply create an alias:

using clock_type = std::chrono::system_clock;

This is a good scenario for an alias because it shortens your typing. More importantly, you have defined what clock type you use in one specific place; this allows you to change the clock type at one spot and have it affect your entire class.

You could even make it a template parameter if you want to allow users to provide their own clock type.


###Constructor

I'm not sure why you initialize _start inside the constructor body instead of the constructor initializer list.

CSDSTimer(std::string name, std::string context = "global",
    std::ostream &stream = std::cout)
    : _name(name)
    , _context(context)
    , _default_str(&stream)
    , _start{ clock_type::now() } // init here instead of ctor body; note alias usage
{}

Additionally, it might be cleaner if you add some new lines like I did, but in all honesty it's not that hard to read... so this one's up to you.


###Destructor

Here, we notice that your only usage of _end is in the destructor. You don't provide any query member functions, so let's just go ahead and remove _end altogether. This will make the class smaller.

You also want milliseconds, but first take seconds and then multiply by 1000. You can directly get milliseconds by using std::chrono::duration_cast().

~CSDSTimer()
{
    // we can directly compute the elapsed time without storing `_end`
    auto elapsed_ms = std::chrono::duration_cast<std::chrono::milliseconds>(
            clock_type::now() - _start);
    
    *_default_str << "PROFILING_DATUM(\"" << _name << "\", \"" << _context
        << "\", " << elapsed_ms.count() << ", \"C++\")" // no more multiply by 1000
        << std::endl;
}

There is a small subtle bug you can avoid by doing it like shown above, but I will describe it so you can be on the look out in the future.

The bug is that the elapsed time is stored in a std::chrono::duration<double>, but _end - _start basically returns std::chrono::nanoseconds. How is this an issue? Well, std::chrono::nanoseconds must be represented by a signed integer type of at least 64 bits (see: http://en.cppreference.com/w/cpp/chrono/duration).

While a double does have 64 bits, it's certainly lacking the ability to represent values from the integer point of view. This would result in a loss of precision/erroneous results.

Use aliases

Instead of writing std::chrono::system_clock:: everywhere, simply create an alias:

using clock_type = std::chrono::system_clock;

This is a good scenario for an alias because it shortens your typing. More importantly, you have defined what clock type you use in one specific place; this allows you to change the clock type at one spot and have it affect your entire class.

You could even make it a template parameter if you want to allow users to provide their own clock type.


Constructor

I'm not sure why you initialize _start inside the constructor body instead of the constructor initializer list.

CSDSTimer(std::string name, std::string context = "global",
    std::ostream &stream = std::cout)
    : _name(name)
    , _context(context)
    , _default_str(&stream)
    , _start{ clock_type::now() } // init here instead of ctor body; note alias usage
{}

Additionally, it might be cleaner if you add some new lines like I did, but in all honesty it's not that hard to read... so this one's up to you.


Destructor

Here, we notice that your only usage of _end is in the destructor. You don't provide any query member functions, so let's just go ahead and remove _end altogether. This will make the class smaller.

You also want milliseconds, but first take seconds and then multiply by 1000. You can directly get milliseconds by using std::chrono::duration_cast().

~CSDSTimer()
{
    // we can directly compute the elapsed time without storing `_end`
    auto elapsed_ms = std::chrono::duration_cast<std::chrono::milliseconds>(
            clock_type::now() - _start);
    
    *_default_str << "PROFILING_DATUM(\"" << _name << "\", \"" << _context
        << "\", " << elapsed_ms.count() << ", \"C++\")" // no more multiply by 1000
        << std::endl;
}

There is a small subtle bug you can avoid by doing it like shown above, but I will describe it so you can be on the look out in the future.

The bug is that the elapsed time is stored in a std::chrono::duration<double>, but _end - _start basically returns std::chrono::nanoseconds. How is this an issue? Well, std::chrono::nanoseconds must be represented by a signed integer type of at least 64 bits (see: http://en.cppreference.com/w/cpp/chrono/duration).

While a double does have 64 bits, it's certainly lacking the ability to represent values from the integer point of view. This would result in a loss of precision/erroneous results.

added 596 characters in body
Source Link
user2296177
  • 3.6k
  • 1
  • 16
  • 37

###Use aliases

Instead of writing std::chrono::system_clock:: everywhere, simply create an alias:

using clock_type = std::chrono::system_clock;

This is a good scenario for an alias because it shortens your typing. More importantly, you have defined what clock type you use in one specific place; this allows you to change the clock type at one spot and have it affect your entire class.

You could even make it a template parameter if you want to allow users to provide their own clock type.


###Constructor

I'm not sure why you initialize _start inside the constructor body instead of the constructor initializer list.

CSDSTimer(std::string name, std::string context = "global",
    std::ostream &stream = std::cout)
    : _name(name)
    , _context(context)
    , _default_str(&stream)
    , _start{ clock_type::now() } // init here instead of ctor body; note alias usage
{}

Additionally, it might be cleaner if you add some new lines like I did, but in all honesty it's not that hard to read... so this one's up to you.


###Destructor

Here, we notice that your only usage of _end is in the destructor. You don't provide any query member functions, so let's just go ahead and remove _end altogether. This will make the class smaller.

You also want milliseconds, but first take seconds and then multiply by 1000. You can directly get milliseconds by using std::chrono::duration_cast().

~CSDSTimer()
{
    // we can directly compute the elapsed time without storing `_end`
    auto elapsed_ms = std::chrono::duration_cast<std::chrono::milliseconds>(
            clock_type::now() - _start);
    
    *_default_str << "PROFILING_DATUM(\"" << _name << "\", \"" << _context
        << "\", " << elapsed_ms.count() << ", \"C++\")" // no more multiply by 1000
        << std::endl;
}

There is a small subtle bug you can avoid by doing it like shown above, but I will describe it so you can be on the look out in the future.

The bug is that the elapsed time is stored in a std::chrono::duration<double>, but _end - _start basically returns std::chrono::nanoseconds. How is this an issue? Well, std::chrono::nanoseconds must be represented by a signed integer type of at least 64 bits (see: http://en.cppreference.com/w/cpp/chrono/duration).

While a double does have 64 bits, it's certainly lacking the ability to represent values from the integer point of view. This would result in a loss of precision/erroneous results.

###Use aliases

Instead of writing std::chrono::system_clock:: everywhere, simply create an alias:

using clock_type = std::chrono::system_clock;

This is a good scenario for an alias because it shortens your typing. More importantly, you have defined what clock type you use in one specific place; this allows you to change the clock type at one spot and have it affect your entire class.

You could even make it a template parameter if you want to allow users to provide their own clock type.


###Constructor

I'm not sure why you initialize _start inside the constructor body instead of the constructor initializer list.

CSDSTimer(std::string name, std::string context = "global",
    std::ostream &stream = std::cout)
    : _name(name)
    , _context(context)
    , _default_str(&stream)
    , _start{ clock_type::now() } // init here instead of ctor body; note alias usage
{}

Additionally, it might be cleaner if you add some new lines like I did, but in all honesty it's not that hard to read... so this one's up to you.


###Destructor

Here, we notice that your only usage of _end is in the destructor. You don't provide any query member functions, so let's just go ahead and remove _end altogether. This will make the class smaller.

You also want milliseconds, but first take seconds and then multiply by 1000. You can directly get milliseconds by using std::chrono::duration_cast().

~CSDSTimer()
{
    // we can directly compute the elapsed time without storing `_end`
    auto elapsed_ms = std::chrono::duration_cast<std::chrono::milliseconds>(
            clock_type::now() - _start);
    
    *_default_str << "PROFILING_DATUM(\"" << _name << "\", \"" << _context
        << "\", " << elapsed_ms.count() << ", \"C++\")" // no more multiply by 1000
        << std::endl;
}

###Use aliases

Instead of writing std::chrono::system_clock:: everywhere, simply create an alias:

using clock_type = std::chrono::system_clock;

This is a good scenario for an alias because it shortens your typing. More importantly, you have defined what clock type you use in one specific place; this allows you to change the clock type at one spot and have it affect your entire class.

You could even make it a template parameter if you want to allow users to provide their own clock type.


###Constructor

I'm not sure why you initialize _start inside the constructor body instead of the constructor initializer list.

CSDSTimer(std::string name, std::string context = "global",
    std::ostream &stream = std::cout)
    : _name(name)
    , _context(context)
    , _default_str(&stream)
    , _start{ clock_type::now() } // init here instead of ctor body; note alias usage
{}

Additionally, it might be cleaner if you add some new lines like I did, but in all honesty it's not that hard to read... so this one's up to you.


###Destructor

Here, we notice that your only usage of _end is in the destructor. You don't provide any query member functions, so let's just go ahead and remove _end altogether. This will make the class smaller.

You also want milliseconds, but first take seconds and then multiply by 1000. You can directly get milliseconds by using std::chrono::duration_cast().

~CSDSTimer()
{
    // we can directly compute the elapsed time without storing `_end`
    auto elapsed_ms = std::chrono::duration_cast<std::chrono::milliseconds>(
            clock_type::now() - _start);
    
    *_default_str << "PROFILING_DATUM(\"" << _name << "\", \"" << _context
        << "\", " << elapsed_ms.count() << ", \"C++\")" // no more multiply by 1000
        << std::endl;
}

There is a small subtle bug you can avoid by doing it like shown above, but I will describe it so you can be on the look out in the future.

The bug is that the elapsed time is stored in a std::chrono::duration<double>, but _end - _start basically returns std::chrono::nanoseconds. How is this an issue? Well, std::chrono::nanoseconds must be represented by a signed integer type of at least 64 bits (see: http://en.cppreference.com/w/cpp/chrono/duration).

While a double does have 64 bits, it's certainly lacking the ability to represent values from the integer point of view. This would result in a loss of precision/erroneous results.

Source Link
user2296177
  • 3.6k
  • 1
  • 16
  • 37

###Use aliases

Instead of writing std::chrono::system_clock:: everywhere, simply create an alias:

using clock_type = std::chrono::system_clock;

This is a good scenario for an alias because it shortens your typing. More importantly, you have defined what clock type you use in one specific place; this allows you to change the clock type at one spot and have it affect your entire class.

You could even make it a template parameter if you want to allow users to provide their own clock type.


###Constructor

I'm not sure why you initialize _start inside the constructor body instead of the constructor initializer list.

CSDSTimer(std::string name, std::string context = "global",
    std::ostream &stream = std::cout)
    : _name(name)
    , _context(context)
    , _default_str(&stream)
    , _start{ clock_type::now() } // init here instead of ctor body; note alias usage
{}

Additionally, it might be cleaner if you add some new lines like I did, but in all honesty it's not that hard to read... so this one's up to you.


###Destructor

Here, we notice that your only usage of _end is in the destructor. You don't provide any query member functions, so let's just go ahead and remove _end altogether. This will make the class smaller.

You also want milliseconds, but first take seconds and then multiply by 1000. You can directly get milliseconds by using std::chrono::duration_cast().

~CSDSTimer()
{
    // we can directly compute the elapsed time without storing `_end`
    auto elapsed_ms = std::chrono::duration_cast<std::chrono::milliseconds>(
            clock_type::now() - _start);
    
    *_default_str << "PROFILING_DATUM(\"" << _name << "\", \"" << _context
        << "\", " << elapsed_ms.count() << ", \"C++\")" // no more multiply by 1000
        << std::endl;
}