Skip to content

Commit

Permalink
Fixed absolute values, correct subsecond width and zero padding
Browse files Browse the repository at this point in the history
  • Loading branch information
brcha committed May 18, 2021
1 parent e2c2704 commit 3bf4aba
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 10 deletions.
39 changes: 29 additions & 10 deletions include/fmt/chrono.h
Original file line number Diff line number Diff line change
Expand Up @@ -405,18 +405,34 @@ inline size_t strftime(wchar_t* str, size_t count, const wchar_t* format,

FMT_END_DETAIL_NAMESPACE

template <typename Char, typename Duration>
struct formatter<std::chrono::time_point<std::chrono::system_clock, Duration>,
template <class Rep, class Period, class = std::enable_if<
std::chrono::duration<Rep, Period>::min() < std::chrono::duration<Rep, Period>::zero()>>
constexpr std::chrono::duration<Rep, Period> abs(std::chrono::duration<Rep, Period> d)
{
return d >= d.zero() ? d : -d;
}

template <typename Char, typename Rep, typename Period>

This comment has been minimized.

Copy link
@ecorm

ecorm May 18, 2021

There needs to be special handling when Rep is floating point (detected via std::chrono::treat_as_floating_point).

struct formatter<std::chrono::time_point<std::chrono::system_clock, std::chrono::duration<Rep, Period>>,
Char> : formatter<std::tm, Char> {
template <typename FormatContext>
auto format(std::chrono::time_point<std::chrono::system_clock> val,
FormatContext& ctx) -> decltype(ctx.out()) {
std::tm time = localtime(val);
auto epoch = val.time_since_epoch();
auto seconds = std::chrono::duration_cast<std::chrono::milliseconds>(epoch).count() / 1000;
auto today = std::chrono::high_resolution_clock::duration(std::chrono::milliseconds(seconds * 1000));
auto subseconds = std::chrono::duration_cast<std::chrono::nanoseconds>(epoch - today);
return formatter<std::tm, Char>::format(time, ctx, subseconds.count());
auto seconds = std::chrono::duration_cast<std::chrono::seconds>(epoch);
auto subseconds = abs(std::chrono::duration_cast<std::chrono::duration<Rep, Period>>(epoch - seconds));

This comment has been minimized.

Copy link
@ecorm

ecorm May 18, 2021

This will not yield correct results for times preceding the epoch.

This comment has been minimized.

Copy link
@ecorm

ecorm May 18, 2021

seconds needs to be computed with the equivalent of std::chrono::floor (added in C++17). Then epoch - seconds will be guaranteed to be positive.

However, I don't know if localtime(val) performs a floor or truncate operation. localtime(val) should be made to floor if it doesn't already do so.

Note: floor rounds towards the past, truncate rounds towards zero (i.e. the epoch). duration_cast does truncation.


if (subseconds.count() > 0) {
auto width = std::to_string(Period::den).size() - 1;

This comment has been minimized.

Copy link
@ecorm

ecorm May 18, 2021

This only works when Period::num is 1, which is not guaranteed. This can be computed at compile time. See how Hinnant does it (it's quite complex, though).

This comment has been minimized.

This comment has been minimized.

std::basic_stringstream<Char> os;
os.fill('0');
os.width(width);
os << subseconds.count();
auto formatted_ss = os.str();
return formatter<std::tm, Char>::format(time, ctx, formatted_ss);
}
return formatter<std::tm, Char>::format(time, ctx);
}
};

Expand All @@ -432,7 +448,7 @@ template <typename Char> struct formatter<std::tm, Char> {
}

template <typename FormatContext>
auto format(const std::tm& tm, FormatContext& ctx, int subseconds = 0) const
auto format(const std::tm& tm, FormatContext& ctx, const std::basic_string<Char> subseconds = {}) const
-> decltype(ctx.out()) {
basic_memory_buffer<Char> tm_format;
tm_format.append(specs.begin(), specs.end());
Expand All @@ -441,8 +457,9 @@ template <typename Char> struct formatter<std::tm, Char> {
// https://github.com/fmtlib/fmt/issues/2238
auto hasS = std::find(tm_format.begin(), tm_format.end(), 'S') != tm_format.end();
auto hasT = std::find(tm_format.begin(), tm_format.end(), 'T') != tm_format.end();
auto writeSubseconds = (hasS || hasT) && (subseconds != 0);
auto writeSubseconds = (hasS || hasT) && (subseconds.size() > 0);
if (writeSubseconds)
// should be std::use_facet<std::numpunct<Char>>(locale).decimal_point();
tm_format.push_back('.');
else
tm_format.push_back(' ');
Expand All @@ -460,12 +477,14 @@ template <typename Char> struct formatter<std::tm, Char> {
buf.reserve(buf.capacity() + (size > MIN_GROWTH ? size : MIN_GROWTH));
}

auto buf_end = buf.end() - 1;
if (writeSubseconds) {
buf.append(std::to_string(subseconds));
buf.append(subseconds);
buf_end = buf.end();
}

// Remove the extra space.
return std::copy(buf.begin(), buf.end() - 1, ctx.out());
return std::copy(buf.begin(), buf_end, ctx.out());
}

basic_string_view<Char> specs;
Expand Down
4 changes: 4 additions & 0 deletions test/chrono-test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ TEST(chrono_test, time_point) {
std::chrono::time_point<std::chrono::system_clock, std::chrono::seconds>;
auto t2 = time_point(std::chrono::seconds(42));
EXPECT_EQ(strftime(t2), fmt::format("{:%Y-%m-%d %H:%M:%S}", t2));
using time_point_2 = std::chrono::time_point<std::chrono::system_clock,
std::chrono::milliseconds>;
auto t3 = time_point_2(std::chrono::milliseconds(1023));
EXPECT_EQ("01.023", fmt::format("{:%S}", t3));

This comment has been minimized.

Copy link
@ecorm

ecorm May 18, 2021

Should also test with time preceding the epoch, as well as whole seconds (zero subseconds).

}

#ifndef FMT_STATIC_THOUSANDS_SEPARATOR
Expand Down

0 comments on commit 3bf4aba

Please sign in to comment.