Skip to content

Parser for chrono::time_point#648

Merged
liuzicheng1987 merged 9 commits intogetml:mainfrom
ron0studios:main
Apr 5, 2026
Merged

Parser for chrono::time_point#648
liuzicheng1987 merged 9 commits intogetml:mainfrom
ron0studios:main

Conversation

@ron0studios
Copy link
Copy Markdown
Contributor

Addresses #586.

There are a couple immediate problems that I am not sure how to proceed with.

This forces an ISO 8601 time format conversion, which also needs some logic to convert. This could be slow.

I believe Parser duration uses a simpler struct format, which makes sense for the type:
{"seconds": 1234567, "microseconds": 123456}, but for a time_point a date-time format would make more sense.

Also we could go a step further and use nanosecond precision instead of the microsecond precision shown here.

TODO

  • timezone support? currently using Z (utc)
  • improve from_string performance
  • docs

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces support for std::chrono::time_point serialization and deserialization, including a new Parser_time_point.hpp header and comprehensive unit tests. The implementation handles ISO 8601 formatting with microsecond precision. Feedback focuses on restricting the parser to std::chrono::system_clock to ensure compatibility with calendar-time conversions, increasing precision to nanoseconds, and improving input validation to strictly enforce the 'Z' suffix. Additionally, a thread-safety improvement was suggested regarding locale handling during parsing.

@ron0studios
Copy link
Copy Markdown
Contributor Author

just timezones/docs left.
The write path at the moment always uses UTC and the read path doesn't work with non UTC timezones. I don't think time_point is timezoned, but on the read path you'll need to convert to UTC which is a loss of data on the timezone.

A parser for std::chrono::zoned_time down the line would fix it, perhaps a follow-up PR instead of this one.

For the time being I'll just do the timezone conversions :)

@ron0studios
Copy link
Copy Markdown
Contributor Author

ron0studios commented Apr 5, 2026

Timezone conversion and docs done, implementing a parser for std::chrono::zoned_time next would let users preserve timezone data when reading/writing. I can work on this later next week.

I think though this may or may not be a slippery slope of many more std::chrono parsers to come? Not sure whats best for reflect-cpp.

Otherwise, this is ready for review :)

@liuzicheng1987

@liuzicheng1987
Copy link
Copy Markdown
Contributor

@ron0studios looks great. No notes. Should we merge?

@ron0studios
Copy link
Copy Markdown
Contributor Author

@liuzicheng1987 Nope i think things look good from my end - ill work on zoned_time as well some point next week

@liuzicheng1987 liuzicheng1987 merged commit 4a54589 into getml:main Apr 5, 2026
182 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants