Rewrite format_http_date with std::chrono#2406
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the old http_date(time_t) API with a chrono‐based format_http_date and updates all callers to use std::chrono::system_clock::time_point.
- Removed
http_date(time_t)overloads and template; addedformat_http_dateoverloads fortime_pointand buffer API. - Rewrote
util.ccimplementations to leverage C++20<chrono>calendar types andhh_mm_ss. - Updated tests and HTTP server code to call the new API.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/util_test.cc | Switched tests from http_date(time_t) to format_http_date. |
| src/util.h | Deleted old http_date declarations; added format_http_date prototypes. |
| src/util.cc | Implemented new chrono‐based format_http_date functions and removed legacy code. |
| src/HttpServer.cc | Updated cached‐date and Last-Modified headers to call format_http_date. |
Comments suppressed due to low confidence (2)
src/util.cc:207
- The buffer is allocated with 29 bytes but the implementation writes a '\0' at index 29. Increase the initial size to 30 to accommodate the null terminator and avoid an out-of-bounds write.
std::string res(29, 0);
src/HttpServer.cc:253
- [nitpick] Instead of manually constructing a
time_pointfromseconds, usestd::chrono::system_clock::from_time_t(tstamp_cached_)for clearer intent and to leverage the standard conversion API.
cached_date_(util::format_http_date(std::chrono::system_clock::time_point{std::chrono::seconds(static_cast<time_t>(tstamp_cached_))})) {
| // Returns given time |tp| in HTTP Date format (e.g., Mon, 10 Oct 2016 | ||
| // 10:25:58 GMT) | ||
| std::string format_http_date(const std::chrono::system_clock::time_point &tp); | ||
|
|
There was a problem hiding this comment.
[nitpick] Consider providing an overload format_http_date(time_t) (or a helper inline auto format_http_date(time_t t) { return format_http_date(std::chrono::system_clock::from_time_t(t)); }) to simplify callers that naturally deal in time_t.
| // Helper function to simplify callers that deal with time_t. | |
| inline auto format_http_date(time_t t) { | |
| return format_http_date(std::chrono::system_clock::from_time_t(t)); | |
| } |
tatsuhiro-t
force-pushed
the
rewrite-format_http_date
branch
from
7732fb6 to
ac080ae
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR replaces the legacy http_date API (which took time_t) with a new format_http_date implementation using std::chrono::system_clock::time_point. It updates the public API in util.h, the implementation in util.cc (with two overloads), adjusts tests to use the new function, and updates all callers in HttpServer.cc.
- Remove old
http_date(time_t)overloads and introduceformat_http_datevariants. - Rewrite
util.ccimplementation to use<chrono>facilities and return bothstd::stringandStringRef. - Update tests and HTTP server code to call the new API.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/util_test.cc | Change tests to use format_http_date and chrono types |
| src/util.h | Remove old declarations and add new format_http_date |
| src/util.cc | Implement new format_http_date (string + buffer API) |
| src/HttpServer.cc | Update all calls from http_date to format_http_date |
Comments suppressed due to low confidence (3)
src/util_test.cc:266
- Using the
"..."sstring literal suffix requires importing the string-literal namespace. Addusing namespace std::literals;(orstd::string_literals) at the top of this file, or remove thessuffix and compare against a plain string literal to ensure the test compiles.
assert_stdstring_equal("Thu, 01 Jan 1970 00:00:00 GMT"s,
src/util.cc:211
- [nitpick] The variable name
sis ambiguous. Consider renaming it to something more descriptive, such asdate_reforref, to clarify that it holds theStringRefreturned from the buffer API.
auto s = format_http_date(res.data(), tp);
src/HttpServer.cc:254
- [nitpick] Casting the floating-point
tstamp_cached_directly totime_twill truncate sub-second data. For consistency with the chrono-based format, consider creating atime_pointand usingstd::chrono::floor<std::chrono::seconds>before formatting.
util::format_http_date(std::chrono::system_clock::from_time_t(static_cast<time_t>(tstamp_cached_))))
| // least 30 bytes, including terminal NULL byte. This function | ||
| // returns StringRef wrapping the buffer pointed by |out|, and this | ||
| // string is terminated by NULL. | ||
| StringRef format_http_date(char *out, |
There was a problem hiding this comment.
[nitpick] The code repeatedly uses the literal size 29 (plus one for NUL). Consider extracting this into a named constant (e.g., constexpr size_t kHttpDateLen = 29;) to avoid magic numbers and keep comments/API docs in sync.
| // least 30 bytes, including terminal NULL byte. This function | |
| // returns StringRef wrapping the buffer pointed by |out|, and this | |
| // string is terminated by NULL. | |
| StringRef format_http_date(char *out, | |
| // least kHttpDateLen + 1 bytes, including terminal NULL byte. This function | |
| // returns StringRef wrapping the buffer pointed by |out|, and this | |
| // string is terminated by NULL. | |
| StringRef format_http_date(char (&out)[kHttpDateLen + 1], |
No description provided.