Both methods have bugs:
The first method takes no account of leap seconds, whereas the second might (depending on the platform).
The second method isn't guaranteed to return its result in units of seconds (although the use of gmtime_r()
, suggests you're targeting POSIX, which does make that guarantee).
I had to infer some definitions, because the review is incomplete (and the linked code was inaccessible when I attempted to access it):
#include <ctime>
#include <utility>
using uint = unsigned int;
I also needed to add a definition of is_leap_year
in the first function:
auto const is_leap_year = [](uint y){ return y%4 ? 0 : y%100 ? 1 : y%400 == 0; };
I could then add a benchmark, and a main()
to run it:
template<typename F>
struct benchmark
{
const F func;
benchmark(F func) : func{std::move(func)} {}
auto operator()() const
{
decltype(func(0,0,0,0,0,0,0,0,0,0,0,0)) sum = 0;
for (auto year = 1900; year <= 2000; ++year)
for (auto month = 1; month <= 12; ++month)
sum += func(year, month, 20, 12, 0, 0,
2000, 6, 1, 8, 0, 0);
return sum;
}
};
#include <chrono>
#include <ostream>
template<typename T>
struct time_printer
{
T func;
friend std::ostream& operator<<(std::ostream& os, const time_printer& p)
{
using Duration
= std::chrono::duration<double, std::chrono::milliseconds::period>;
auto begin = std::chrono::steady_clock::now();
p.func();
auto end = std::chrono::steady_clock::now();
Duration time_taken = end - begin;
return os << time_taken.count();
}
};
template<typename T>
time_printer<T> print_time(T fun) { return {fun}; }
#include <iostream>
int main()
{
std::clog << "Method 1: "
<< print_time(benchmark(calculate_seconds_between)) << std::endl;
std::clog << "Method 2: "
<< print_time(benchmark(calculate_seconds_between2)) << std::endl;
}
This gives me a baseline ratio of about 100 between the two methods (after removing the unnecessary call to std::time
in the second function). std::mktime()
is always likely to be slower, as it must update tm_yday
and (significantly) tm_wday
in the passed structure.
Let's now have a look at the function:
The big if
/else
chain is ugly, as you noticed. The usual fix is to include <tuple>
:
auto t1 = std::tie(Y1, M1, D1, H1, m1, S1),
t2 = std::tie(Y2, M2, D2, H2, m2, S2);
auto invert = t2 < t1;
if (invert) std::swap(t1, t2);
As an alternative to recording invert
, we can simply recurse with the arguments swapped:
auto t1 = std::tie(Y1, M1, D1, H1, m1, S1),
t2 = std::tie(Y2, M2, D2, H2, m2, S2);
if (t2 < t1)
return - calculate_seconds_between(Y2, M2, D2, H2, m2, S2,
Y2, M2, D2, H2, m2, S2);
Both of these changes are below the noise floor in my performance measurements.
We can simplify the calculation of lY1
and lY2
given that we've almost calculated the day-in-year just before. Move the addition of Yn_days
and Dn
, then we can just use that day number:
const uint Y1_days = month_days_sum[M1 - 1] + D1;
const uint Y2_days = month_days_sum[M2 - 1] + D2;
static const uint feb_29th = 60;
const bool lY1 = is_leap_year(Y1) && Y1_days < feb_29th;
const bool lY2 = is_leap_year(Y2) && M2 > 2;
// Time difference in seconds
const long S1s = ((Y1_days) * 86400) + (H1 * 3600) + (m1 * 60) + S1;
const long S2s = ((Y2_days) * 86400) + (H2 * 3600) + (m2 * 60) + S2;
(I fixed the logic for lY2
- it's a shame you didn't include the unit tests, as I think you have missed at least one, with the end date being 29 February).
We can also simplify the calculation of first and last leap day to avoid branching:
years_days += lY1 + lY2 - (Y1 == Y2);
Alternatively, we could just conditionally include the first and/or last year into the loop:
for (uint i = Y1 + (Y1_days < feb_29th); i <= Y2 - (M2 > 2);) {
if (is_leap_year(i)) {
++years_days;
i += 4;
} else {
++i;
}
}
We can remove two of the multiplications by 86400, by summing days first. We can eliminate all the paired multiplications like this:
// compute total seconds
const long days = years_days + Y2_days - Y1_days;
const long hours = days*24 + H2 - H1;
const long minutes = hours*60 + m2 - m1;
return minutes*60 + S2 - S1;
Here, we avoid the subtle bug in the original, where years_days * 86400
(int * int
=> int
) could overflow (remember, INT_MAX
can be as low as 65536) before being widened to
long years_seconds`.
#My version
#include
#include
#include
using uint = unsigned int;
long calculate_seconds_between(
const uint Y1, const uint M1, const uint D1,
const uint H1, const uint m1, const uint S1,
const uint Y2, const uint M2, const uint D2,
const uint H2, const uint m2, const uint S2)
{
auto const
t1 = std::tie(Y1, M1, D1, H1, m1, S1),
t2 = std::tie(Y2, M2, D2, H2, m2, S2);
if (t2 < t1)
return - calculate_seconds_between(Y2, M2, D2, H2, m2, S2,
Y2, M2, D2, H2, m2, S2);
int years_days = (Y2 - Y1) * 365;
// Leap Years
auto const is_leap_year
= [](uint y){ return y%4 ? 0 : y%100 ? 1 : y%400 == 0; };
for (uint i = Y1 + (M1 > 2); i < Y2 + (M2 > 2);) {
if (is_leap_year(i)) {
++years_days;
i += 4;
} else {
++i;
}
}
static const uint month_days_sum[] =
{0, 31, 59, 90, 120, 151, 181, 212, 243, 273, 304, 334, 365};
const uint Y1_days = month_days_sum[M1 - 1] + D1;
const uint Y2_days = month_days_sum[M2 - 1] + D2;
// compute total seconds
const long days = years_days + Y2_days - Y1_days;
const long hours = days*24 + H2 - H1;
const long minutes = hours*60 + m2 - m1;
return minutes*60 + S2 - S1;
}