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).

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.