Skip to content

type-chrono: Add CustomBuildField/CustomReadField overloads for std::chrono::time_point #303

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ryanofsky wants to merge 1 commit into
base: master
Choose a base branch
from
Open

type-chrono: Add CustomBuildField/CustomReadField overloads for std::chrono::time_point #303

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions include/mp/type-chrono.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,29 @@ decltype(auto) CustomReadField(TypeList<std::chrono::duration<Rep, Period>>, Pri
{
return read_dest.construct(input.get());
}

//! Overload CustomBuildField and CustomReadField to serialize
//! std::chrono::time_point parameters and return values as integer tick counts.
//! The capnp field type must be an integer type with enough range to hold the
//! time_since_epoch() count for the given Duration (e.g. Int64 for nanoseconds).
template <class Clock, class Duration, typename Value, typename Output>
void CustomBuildField(TypeList<std::chrono::time_point<Clock, Duration>>, Priority<1>, InvokeContext& invoke_context,
Value&& value, Output&& output)
{
using Rep = typename Duration::rep;
static_assert(std::numeric_limits<decltype(output.get())>::lowest() <= std::numeric_limits<Rep>::lowest(),

@ViniciusCestarii ViniciusCestarii Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The range checks in both CustomBuildField overloads have a signedness bug:

static_assert(std::numeric_limits<decltype(output.get())>::lowest() <= std::numeric_limits<Rep>::lowest(), ...);

When the capnp field type is unsigned (e.g. UInt64) and Rep is signed (e.g. int64_t for std::chrono::nanoseconds), the usual arithmetic conversions convert Rep::lowest() (INT64_MIN) to the unsigned type before comparing, turning it into a huge positive number. So 0 <= INT64_MIN silently evaluates to true, and the assert passes for a field that can't actually represent negative tick counts (e.g. any pre-epoch system_clock::time_point).

Concretely, this means a UInt64 field paired with a signed Duration::rep compiles today, and output.set(negative_count) wraps the value into UINT64_MAX. That's silently wrong for anything that reads the field as its declared (unsigned) type — a different language's capnp bindings, a JSON dump, or a raw comparison would see 18446744073709551615 instead of -1. A C++ round trip through the same chrono type can look fine, since the wrap is bit-for-bit invertible at equal width, which is what makes this easy to miss in testing.

Fix: use std::cmp_less_equal/std::cmp_greater_equal (C++20, ) instead of <=/>=, since they're designed to compare integers of differing signedness correctly:

static_assert(std::cmp_less_equal(std::numeric_limits<decltype(output.get())>::lowest(), std::numeric_limits<Rep>::lowest()),
              "capnp type does not have enough range to hold lowest std::chrono::time_point value");
static_assert(std::cmp_greater_equal(std::numeric_limits<decltype(output.get())>::max(), std::numeric_limits<Rep>::max()),
              "capnp type does not have enough range to hold highest std::chrono::time_point value");

I verified this by temporarily declaring a test field as UInt64 against a signed Rep: it compiled before the fix and correctly fails to compile after.

Happy to push chrono type tests as a follow-up PR if useful.

Note: I had AI help me write this up clearly, apologies if the phrasing feels more polished or sloppy than my usual comments.

@ryanofsky ryanofsky Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: #303 (comment)

Good catch! I think this makes sense and it looks like this same problem also exists other places: for durations above and also in type-number.h. I'll see if it's possible to fix them all here without breaking anything.

"capnp type does not have enough range to hold lowest std::chrono::time_point value");
static_assert(std::numeric_limits<decltype(output.get())>::max() >= std::numeric_limits<Rep>::max(),
"capnp type does not have enough range to hold highest std::chrono::time_point value");
output.set(value.time_since_epoch().count());
}

template <class Clock, class Duration, typename Input, typename ReadDest>
decltype(auto) CustomReadField(TypeList<std::chrono::time_point<Clock, Duration>>, Priority<1>,
InvokeContext& invoke_context, Input&& input, ReadDest&& read_dest)
{
return read_dest.construct(std::chrono::time_point<Clock, Duration>{Duration{input.get()}});
}
} // namespace mp

#endif // MP_PROXY_TYPE_CHRONO_H
Loading