[libc] create TimeReader to look at a struct tm #126138
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the process of adding strftime (llvm#122556) I wrote this utility class to simplify reading from a struct tm. It provides helper functions that return basically everything needed by strftime. It's not tested directly, but it is thoroughly exercised by the strftime tests.
|
@llvm/pr-subscribers-libc Author: Michael Jones (michaelrj-google) ChangesIn the process of adding strftime (#122556) I wrote this utility class Patch is 21.50 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/126138.diff 6 Files Affected:
diff --git a/libc/include/llvm-libc-types/struct_tm.h b/libc/include/llvm-libc-types/struct_tm.h
index 9fef7c5718ea4a5..2ec74ecac0293b6 100644
--- a/libc/include/llvm-libc-types/struct_tm.h
+++ b/libc/include/llvm-libc-types/struct_tm.h
@@ -19,6 +19,7 @@ struct tm {
int tm_wday; // days since Sunday
int tm_yday; // days since January
int tm_isdst; // Daylight Saving Time flag
+ // TODO: add tm_gmtoff and tm_zone? (posix extensions)
};
#endif // LLVM_LIBC_TYPES_STRUCT_TM_H
diff --git a/libc/src/time/CMakeLists.txt b/libc/src/time/CMakeLists.txt
index ef9bfe57bc4ec29..dd28aa67280b7f3 100644
--- a/libc/src/time/CMakeLists.txt
+++ b/libc/src/time/CMakeLists.txt
@@ -22,6 +22,8 @@ add_object_library(
DEPENDS
libc.include.time
libc.src.__support.CPP.limits
+ libc.src.__support.CPP.string_view
+ libc.src.__support.CPP.optional
libc.src.errno.errno
.time_constants
libc.hdr.types.time_t
diff --git a/libc/src/time/mktime.cpp b/libc/src/time/mktime.cpp
index 3874cad02facbd6..fc05ff2930434f9 100644
--- a/libc/src/time/mktime.cpp
+++ b/libc/src/time/mktime.cpp
@@ -14,100 +14,8 @@
namespace LIBC_NAMESPACE_DECL {
-// Returns number of years from (1, year).
-static constexpr int64_t get_num_of_leap_years_before(int64_t year) {
- return (year / 4) - (year / 100) + (year / 400);
-}
-
-// Returns True if year is a leap year.
-static constexpr bool is_leap_year(const int64_t year) {
- return (((year) % 4) == 0 && (((year) % 100) != 0 || ((year) % 400) == 0));
-}
-
LLVM_LIBC_FUNCTION(time_t, mktime, (struct tm * tm_out)) {
- // Unlike most C Library functions, mktime doesn't just die on bad input.
- // TODO(rtenneti); Handle leap seconds.
- int64_t tm_year_from_base = tm_out->tm_year + time_constants::TIME_YEAR_BASE;
-
- // 32-bit end-of-the-world is 03:14:07 UTC on 19 January 2038.
- if (sizeof(time_t) == 4 &&
- tm_year_from_base >= time_constants::END_OF32_BIT_EPOCH_YEAR) {
- if (tm_year_from_base > time_constants::END_OF32_BIT_EPOCH_YEAR)
- return time_utils::out_of_range();
- if (tm_out->tm_mon > 0)
- return time_utils::out_of_range();
- if (tm_out->tm_mday > 19)
- return time_utils::out_of_range();
- else if (tm_out->tm_mday == 19) {
- if (tm_out->tm_hour > 3)
- return time_utils::out_of_range();
- else if (tm_out->tm_hour == 3) {
- if (tm_out->tm_min > 14)
- return time_utils::out_of_range();
- else if (tm_out->tm_min == 14) {
- if (tm_out->tm_sec > 7)
- return time_utils::out_of_range();
- }
- }
- }
- }
-
- // Years are ints. A 32-bit year will fit into a 64-bit time_t.
- // A 64-bit year will not.
- static_assert(
- sizeof(int) == 4,
- "ILP64 is unimplemented. This implementation requires 32-bit integers.");
-
- // Calculate number of months and years from tm_mon.
- int64_t month = tm_out->tm_mon;
- if (month < 0 || month >= time_constants::MONTHS_PER_YEAR - 1) {
- int64_t years = month / 12;
- month %= 12;
- if (month < 0) {
- years--;
- month += 12;
- }
- tm_year_from_base += years;
- }
- bool tm_year_is_leap = is_leap_year(tm_year_from_base);
-
- // Calculate total number of days based on the month and the day (tm_mday).
- int64_t total_days = tm_out->tm_mday - 1;
- for (int64_t i = 0; i < month; ++i)
- total_days += time_constants::NON_LEAP_YEAR_DAYS_IN_MONTH[i];
- // Add one day if it is a leap year and the month is after February.
- if (tm_year_is_leap && month > 1)
- total_days++;
-
- // Calculate total numbers of days based on the year.
- total_days += (tm_year_from_base - time_constants::EPOCH_YEAR) *
- time_constants::DAYS_PER_NON_LEAP_YEAR;
- if (tm_year_from_base >= time_constants::EPOCH_YEAR) {
- total_days += get_num_of_leap_years_before(tm_year_from_base - 1) -
- get_num_of_leap_years_before(time_constants::EPOCH_YEAR);
- } else if (tm_year_from_base >= 1) {
- total_days -= get_num_of_leap_years_before(time_constants::EPOCH_YEAR) -
- get_num_of_leap_years_before(tm_year_from_base - 1);
- } else {
- // Calculate number of leap years until 0th year.
- total_days -= get_num_of_leap_years_before(time_constants::EPOCH_YEAR) -
- get_num_of_leap_years_before(0);
- if (tm_year_from_base <= 0) {
- total_days -= 1; // Subtract 1 for 0th year.
- // Calculate number of leap years until -1 year
- if (tm_year_from_base < 0) {
- total_days -= get_num_of_leap_years_before(-tm_year_from_base) -
- get_num_of_leap_years_before(1);
- }
- }
- }
-
- // TODO: https://github.com/llvm/llvm-project/issues/121962
- // Need to handle timezone and update of tm_isdst.
- int64_t seconds = tm_out->tm_sec +
- tm_out->tm_min * time_constants::SECONDS_PER_MIN +
- tm_out->tm_hour * time_constants::SECONDS_PER_HOUR +
- total_days * time_constants::SECONDS_PER_DAY;
+ int64_t seconds = time_utils::mktime_internal(tm_out);
// Update the tm structure's year, month, day, etc. from seconds.
if (time_utils::update_from_seconds(seconds, tm_out) < 0)
diff --git a/libc/src/time/time_constants.h b/libc/src/time/time_constants.h
index 3e25f741745ab53..ab17862fdd957b9 100644
--- a/libc/src/time/time_constants.h
+++ b/libc/src/time/time_constants.h
@@ -18,7 +18,7 @@ namespace LIBC_NAMESPACE_DECL {
namespace time_constants {
enum Month : int {
- JANUARY,
+ JANUARY = 0,
FEBRUARY,
MARCH,
APRIL,
@@ -32,6 +32,16 @@ enum Month : int {
DECEMBER
};
+enum WeekDay : int {
+ SUNDAY = 0,
+ MONDAY,
+ TUESDAY,
+ WEDNESDAY,
+ THURSDAY,
+ FRIDAY,
+ SATURDAY
+};
+
constexpr int SECONDS_PER_MIN = 60;
constexpr int MINUTES_PER_HOUR = 60;
constexpr int HOURS_PER_DAY = 24;
@@ -40,6 +50,9 @@ constexpr int MONTHS_PER_YEAR = 12;
constexpr int DAYS_PER_NON_LEAP_YEAR = 365;
constexpr int DAYS_PER_LEAP_YEAR = 366;
+constexpr int LAST_DAY_OF_NON_LEAP_YEAR = DAYS_PER_NON_LEAP_YEAR - 1;
+constexpr int LAST_DAY_OF_LEAP_YEAR = DAYS_PER_LEAP_YEAR - 1;
+
constexpr int SECONDS_PER_HOUR = SECONDS_PER_MIN * MINUTES_PER_HOUR;
constexpr int SECONDS_PER_DAY = SECONDS_PER_HOUR * HOURS_PER_DAY;
constexpr int NUMBER_OF_SECONDS_IN_LEAP_YEAR =
@@ -49,6 +62,8 @@ constexpr int TIME_YEAR_BASE = 1900;
constexpr int EPOCH_YEAR = 1970;
constexpr int EPOCH_WEEK_DAY = 4;
+constexpr int ISO_FIRST_DAY_OF_YEAR = 3; // the 4th day of the year, 0-indexed.
+
// For asctime the behavior is undefined if struct tm's tm_wday or tm_mon are
// not within the normal ranges as defined in <time.h>, or if struct tm's
// tm_year exceeds {INT_MAX}-1990, or if the below asctime_internal algorithm
diff --git a/libc/src/time/time_utils.cpp b/libc/src/time/time_utils.cpp
index abc93b8cb961ed7..585cae0fc33ce6a 100644
--- a/libc/src/time/time_utils.cpp
+++ b/libc/src/time/time_utils.cpp
@@ -15,6 +15,98 @@
namespace LIBC_NAMESPACE_DECL {
namespace time_utils {
+// TODO: clean this up in a followup patch
+int64_t mktime_internal(const struct tm *tm_out) {
+ // Unlike most C Library functions, mktime doesn't just die on bad input.
+ // TODO(rtenneti); Handle leap seconds.
+ int64_t tm_year_from_base = tm_out->tm_year + time_constants::TIME_YEAR_BASE;
+
+ // 32-bit end-of-the-world is 03:14:07 UTC on 19 January 2038.
+ if (sizeof(time_t) == 4 &&
+ tm_year_from_base >= time_constants::END_OF32_BIT_EPOCH_YEAR) {
+ if (tm_year_from_base > time_constants::END_OF32_BIT_EPOCH_YEAR)
+ return time_utils::out_of_range();
+ if (tm_out->tm_mon > 0)
+ return time_utils::out_of_range();
+ if (tm_out->tm_mday > 19)
+ return time_utils::out_of_range();
+ else if (tm_out->tm_mday == 19) {
+ if (tm_out->tm_hour > 3)
+ return time_utils::out_of_range();
+ else if (tm_out->tm_hour == 3) {
+ if (tm_out->tm_min > 14)
+ return time_utils::out_of_range();
+ else if (tm_out->tm_min == 14) {
+ if (tm_out->tm_sec > 7)
+ return time_utils::out_of_range();
+ }
+ }
+ }
+ }
+
+ // Years are ints. A 32-bit year will fit into a 64-bit time_t.
+ // A 64-bit year will not.
+ static_assert(
+ sizeof(int) == 4,
+ "ILP64 is unimplemented. This implementation requires 32-bit integers.");
+
+ // Calculate number of months and years from tm_mon.
+ int64_t month = tm_out->tm_mon;
+ if (month < 0 || month >= time_constants::MONTHS_PER_YEAR - 1) {
+ int64_t years = month / 12;
+ month %= 12;
+ if (month < 0) {
+ years--;
+ month += 12;
+ }
+ tm_year_from_base += years;
+ }
+ bool tm_year_is_leap = time_utils::is_leap_year(tm_year_from_base);
+
+ // Calculate total number of days based on the month and the day (tm_mday).
+ int64_t total_days = tm_out->tm_mday - 1;
+ for (int64_t i = 0; i < month; ++i)
+ total_days += time_constants::NON_LEAP_YEAR_DAYS_IN_MONTH[i];
+ // Add one day if it is a leap year and the month is after February.
+ if (tm_year_is_leap && month > 1)
+ total_days++;
+
+ // Calculate total numbers of days based on the year.
+ total_days += (tm_year_from_base - time_constants::EPOCH_YEAR) *
+ time_constants::DAYS_PER_NON_LEAP_YEAR;
+ if (tm_year_from_base >= time_constants::EPOCH_YEAR) {
+ total_days +=
+ time_utils::get_num_of_leap_years_before(tm_year_from_base - 1) -
+ time_utils::get_num_of_leap_years_before(time_constants::EPOCH_YEAR);
+ } else if (tm_year_from_base >= 1) {
+ total_days -=
+ time_utils::get_num_of_leap_years_before(time_constants::EPOCH_YEAR) -
+ time_utils::get_num_of_leap_years_before(tm_year_from_base - 1);
+ } else {
+ // Calculate number of leap years until 0th year.
+ total_days -=
+ time_utils::get_num_of_leap_years_before(time_constants::EPOCH_YEAR) -
+ time_utils::get_num_of_leap_years_before(0);
+ if (tm_year_from_base <= 0) {
+ total_days -= 1; // Subtract 1 for 0th year.
+ // Calculate number of leap years until -1 year
+ if (tm_year_from_base < 0) {
+ total_days -=
+ time_utils::get_num_of_leap_years_before(-tm_year_from_base) -
+ time_utils::get_num_of_leap_years_before(1);
+ }
+ }
+ }
+
+ // TODO: https://github.com/llvm/llvm-project/issues/121962
+ // Need to handle timezone and update of tm_isdst.
+ int64_t seconds = tm_out->tm_sec +
+ tm_out->tm_min * time_constants::SECONDS_PER_MIN +
+ tm_out->tm_hour * time_constants::SECONDS_PER_HOUR +
+ total_days * time_constants::SECONDS_PER_DAY;
+ return seconds;
+}
+
static int64_t computeRemainingYears(int64_t daysPerYears,
int64_t quotientYears,
int64_t *remainingDays) {
diff --git a/libc/src/time/time_utils.h b/libc/src/time/time_utils.h
index 5e0a692d4db0486..f8521b0e8ec33cd 100644
--- a/libc/src/time/time_utils.h
+++ b/libc/src/time/time_utils.h
@@ -12,6 +12,8 @@
#include "hdr/types/size_t.h"
#include "hdr/types/struct_tm.h"
#include "hdr/types/time_t.h"
+#include "src/__support/CPP/optional.h"
+#include "src/__support/CPP/string_view.h"
#include "src/__support/common.h"
#include "src/__support/macros/config.h"
#include "src/errno/libc_errno.h"
@@ -22,6 +24,10 @@
namespace LIBC_NAMESPACE_DECL {
namespace time_utils {
+// calculates the seconds from the epoch for tm_in. Does not update the struct,
+// you must call update_from_seconds for that.
+int64_t mktime_internal(const struct tm *tm_out);
+
// Update the "tm" structure's year, month, etc. members from seconds.
// "total_seconds" is the number of seconds since January 1st, 1970.
extern int64_t update_from_seconds(int64_t total_seconds, struct tm *tm);
@@ -61,6 +67,7 @@ LIBC_INLINE char *asctime(const struct tm *timeptr, char *buffer,
}
// TODO(michaelr): move this to use the strftime machinery
+ // equivalent to strftime(buffer, bufferLength, "%a %b %T %Y\n", timeptr)
int written_size = __builtin_snprintf(
buffer, bufferLength, "%.3s %.3s%3d %.2d:%.2d:%.2d %d\n",
time_constants::WEEK_DAY_NAMES[timeptr->tm_wday].data(),
@@ -94,6 +101,255 @@ LIBC_INLINE struct tm *localtime(const time_t *t_ptr) {
return time_utils::gmtime_internal(t_ptr, &result);
}
+// Returns number of years from (1, year).
+LIBC_INLINE constexpr int64_t get_num_of_leap_years_before(int64_t year) {
+ return (year / 4) - (year / 100) + (year / 400);
+}
+
+// Returns True if year is a leap year.
+LIBC_INLINE constexpr bool is_leap_year(const int64_t year) {
+ return (((year) % 4) == 0 && (((year) % 100) != 0 || ((year) % 400) == 0));
+}
+
+LIBC_INLINE constexpr int get_days_in_year(const int year) {
+ return is_leap_year(year) ? time_constants::DAYS_PER_LEAP_YEAR
+ : time_constants::DAYS_PER_NON_LEAP_YEAR;
+}
+
+// This is a helper class that takes a struct tm and lets you inspect its
+// values. Where relevant, results are bounds checked and returned as optionals.
+// This class does not, however, do data normalization except where necessary.
+// It will faithfully return a date of 9999-99-99, even though that makes no
+// sense.
+class TMReader final {
+ const tm *timeptr;
+
+public:
+ LIBC_INLINE constexpr TMReader(const tm *tmptr) : timeptr(tmptr) { ; }
+
+ // Strings
+ LIBC_INLINE constexpr cpp::optional<cpp::string_view>
+ get_weekday_short_name() const {
+ if (timeptr->tm_wday >= 0 &&
+ timeptr->tm_wday < time_constants::DAYS_PER_WEEK)
+ return time_constants::WEEK_DAY_NAMES[timeptr->tm_wday];
+
+ return cpp::nullopt;
+ }
+
+ LIBC_INLINE constexpr cpp::optional<cpp::string_view>
+ get_weekday_full_name() const {
+ if (timeptr->tm_wday >= 0 &&
+ timeptr->tm_wday < time_constants::DAYS_PER_WEEK)
+ return time_constants::WEEK_DAY_FULL_NAMES[timeptr->tm_wday];
+
+ return cpp::nullopt;
+ }
+
+ LIBC_INLINE constexpr cpp::optional<cpp::string_view>
+ get_month_short_name() const {
+ if (timeptr->tm_mon >= 0 &&
+ timeptr->tm_mon < time_constants::MONTHS_PER_YEAR)
+ return time_constants::MONTH_NAMES[timeptr->tm_mon];
+
+ return cpp::nullopt;
+ }
+
+ LIBC_INLINE constexpr cpp::optional<cpp::string_view>
+ get_month_full_name() const {
+ if (timeptr->tm_mon >= 0 &&
+ timeptr->tm_mon < time_constants::MONTHS_PER_YEAR)
+ return time_constants::MONTH_FULL_NAMES[timeptr->tm_mon];
+
+ return cpp::nullopt;
+ }
+
+ LIBC_INLINE constexpr cpp::string_view get_am_pm() const {
+ if (timeptr->tm_hour < 12)
+ return "AM";
+ return "PM";
+ }
+
+ LIBC_INLINE constexpr cpp::string_view get_timezone_name() const {
+ // TODO: timezone support
+ return "UTC";
+ }
+
+ // Numbers
+ LIBC_INLINE constexpr int get_sec() const { return timeptr->tm_sec; }
+ LIBC_INLINE constexpr int get_min() const { return timeptr->tm_min; }
+ LIBC_INLINE constexpr int get_hour() const { return timeptr->tm_hour; }
+ LIBC_INLINE constexpr int get_mday() const { return timeptr->tm_mday; }
+ LIBC_INLINE constexpr int get_mon() const { return timeptr->tm_mon; }
+ LIBC_INLINE constexpr int get_yday() const { return timeptr->tm_yday; }
+ LIBC_INLINE constexpr int get_wday() const { return timeptr->tm_wday; }
+ LIBC_INLINE constexpr int get_isdst() const { return timeptr->tm_isdst; }
+
+ // returns the year, counting from 1900
+ LIBC_INLINE constexpr int get_year_raw() const { return timeptr->tm_year; }
+ // returns the year, counting from 0
+ LIBC_INLINE constexpr int get_year() const {
+ return timeptr->tm_year + time_constants::TIME_YEAR_BASE;
+ }
+
+ LIBC_INLINE constexpr int is_leap_year() const {
+ return time_utils::is_leap_year(get_year());
+ }
+
+ LIBC_INLINE constexpr int get_iso_wday() const {
+ // ISO uses a week that starts on Monday, but struct tm starts its week on
+ // Sunday. This function normalizes the weekday so that it always returns a
+ // value 0-6
+ const int NORMALIZED_WDAY =
+ timeptr->tm_wday % time_constants::DAYS_PER_WEEK;
+ return (NORMALIZED_WDAY + (time_constants::DAYS_PER_WEEK - 1)) % 7;
+ }
+
+ // returns the week of the current year, with weeks starting on start_day.
+ LIBC_INLINE constexpr int get_week(time_constants::WeekDay start_day) const {
+ // The most recent start_day. The rest of the days into the current week
+ // don't count, so ignore them.
+ // Also add 7 to handle start_day > tm_wday
+ const int start_of_cur_week =
+ timeptr->tm_yday -
+ ((timeptr->tm_wday + time_constants::DAYS_PER_WEEK - start_day) %
+ time_constants::DAYS_PER_WEEK);
+
+ // Add 1 since the first week may start with day 0
+ const int ceil_weeks_since_start =
+ ((start_of_cur_week + 1) + (time_constants::DAYS_PER_WEEK - 1)) /
+ time_constants::DAYS_PER_WEEK;
+ return ceil_weeks_since_start;
+ }
+
+ LIBC_INLINE constexpr int get_iso_week() const {
+ const time_constants::WeekDay start_day = time_constants::MONDAY;
+
+ // The most recent start_day. The rest of the days into the current week
+ // don't count, so ignore them.
+ // Also add 7 to handle start_day > tm_wday
+ const int start_of_cur_week =
+ timeptr->tm_yday -
+ ((timeptr->tm_wday + time_constants::DAYS_PER_WEEK - start_day) %
+ time_constants::DAYS_PER_WEEK);
+
+ // if the week starts in the previous year, and also if the 4th of this year
+ // is not in this week.
+ if (start_of_cur_week < -3) {
+ const int days_into_prev_year =
+ get_days_in_year(get_year() - 1) + start_of_cur_week;
+ // Each year has at least 52 weeks, but a year's last week will be 53 if
+ // its first week starts in the previous year and its last week ends
+ // in the next year. We know get_year() - 1 must extend into get_year(),
+ // so here we check if it also extended into get_year() - 2 and add 1 week
+ // if it does.
+ return 52 + ((days_into_prev_year % time_constants::DAYS_PER_WEEK) >
+ time_constants::ISO_FIRST_DAY_OF_YEAR
+ ? 1
+ : 0);
+ }
+
+ // subtract 1 to account for yday being 0 indexed
+ const int days_until_end_of_year =
+ get_days_in_year(get_year()) - start_of_cur_week - 1;
+
+ // if there are less than 3 days from the start of this week to the end of
+ // the year, then there must be 4 days in this week in the next year, which
+ // means that this week is the first week of that year.
+ if (days_until_end_of_year < 3)
+ return 1;
+
+ // else just calculate the current week like normal.
+ const int ceil_weeks_since_start =
+ ((start_of_cur_week + 1) + (time_constants::DAYS_PER_WEEK - 1)) /
+ time_constants::DAYS_PER_WEEK;
+
+ // add 1 if this year's first week starts in the previous year.
+ return ceil_weeks_since_start +
+ (((start_of_cur_week + time_constants::DAYS_PER_WEEK) %
+ time_constants::DAYS_PER_WEEK) >
+ time_constants::ISO_FIRST_DAY_OF_YEAR
+ ? 1
+ : 0);
+ }
+
+ LIBC_INLINE constexpr int get_iso_year() const {
+ const int BASE_YEAR = get_year();
+ // The ISO year is the same as a standard year for all dates after the start
+ // of the first week and before the last week. Since the first ISO week of a
+ // year starts on the 4th, anything after that is in this year.
+ if (timeptr->tm_yday >= time_constants::ISO_FIRST_DAY_OF_YEAR &&
+ timeptr->tm_yday < time_constants::DAYS_PER_NON_LEAP_YEAR -
+ time_constants::DAYS_PER_WEEK)
+ return BASE_YEAR;
+
+ const int ISO_WDAY = get_iso_wday();
+ // The first week of the ISO year is defined as the week containing the
+ // 4th day of January.
+
+ // first week
+ if (timeptr->tm_yday < time_constants::ISO_FIRST_DAY_OF_YEAR) {
+ /*
+ If jan 4 is in this week, then we're in BASE_YEAR, else we're in the
+ previous year. The formula's been rearranged so here's the derivation:
+
+ +--------+-- days until jan 4
+ | |
+ wday + (4 - yday) < 7
+ | |
+ +---------------+-- weekday of jan 4
+
+ rearranged to get all the constants on one side:
+
+ wday - ...
[truncated]
|
libc/src/time/time_utils.h
Outdated
Comment on lines
203
to
205
| const int NORMALIZED_WDAY = | ||
| timeptr->tm_wday % time_constants::DAYS_PER_WEEK; | ||
| return (NORMALIZED_WDAY + (time_constants::DAYS_PER_WEEK - 1)) % 7; |
There was a problem hiding this comment.
I thought we use ALL_CAPS identifiers for constexpr? NORMALIZED_WDAY isn't constexpr.
There was a problem hiding this comment.
It doesn't look like we have a specific case for just const variables in libc/.clang_tidy. I generally try to use all caps for things that I can't change, so constexpr but also const. I'm not particularly picky here but we should probably look into fixing this as a followup
libc/src/time/time_utils.h
Outdated
Comment on lines
246
to
249
| return 52 + ((days_into_prev_year % time_constants::DAYS_PER_WEEK) > | ||
| time_constants::ISO_FIRST_DAY_OF_YEAR | ||
| ? 1 | ||
| : 0); |
There was a problem hiding this comment.
The type of an expression in the form of x > y evaluates to int in C and bool in C++. But C++ will still add an implicit cast to int when used as a subexpression to one side of binary +.
So I think you can omit the ternary expression in the form + (x > y ? 1 : 0) and replace it with + (x > y), here and below, if you want.
There was a problem hiding this comment.
Personally I generally prefer to avoid the implicit cast from bool to int, but here it does simplify the code so I have changed it as suggested.
libc/src/time/time_utils.h
Outdated
Comment on lines
133
to
134
| if (timeptr->tm_wday >= 0 && | ||
| timeptr->tm_wday < time_constants::DAYS_PER_WEEK) |
There was a problem hiding this comment.
I guess this is shorter than
Suggested change
| if (timeptr->tm_wday >= 0 && | |
| timeptr->tm_wday < time_constants::DAYS_PER_WEEK) | |
| if (timeptr->tm_wday >= 0 && | |
| timeptr->tm_wday < time_constants::WEEK_DAY_NAMES.size()) |
but I think you could further share the bounds check and have less code:
cpp::string_view bounds_check(const cpp::span<cpp::string_view> &arr, int index) {
return index >= 0 && index < arr.size() ? arr[index] : {};
}
cpp::string_view get_weekday_short_name() const {
return bounds_check(time_constants::WEEK_DAY_NAMES, timeptr->tm_wday);
}
cpp::string_view get_weekday_full_name() const {
return bounds_check(time_constants::WEEK_DAY_FULL_NAMES, timeptr->tm_wday);
}
cpp::string_view get_month_short_name const {
return bounds_check(time_constants::MONTH_NAMES, timeptr->tm_mon);
}
cpp::string_view get_month_full_name const {
return bounds_check(time_constants::MONTH_FULL_NAMES, timeptr->tm_mon);
}but perhaps our cpp::array doesn't (yet) convert to cpp::span?
There was a problem hiding this comment.
I wrote something similar, but not quite the same. Basically, instead of converting to a span I just templated the input array by size. Since it's getting inlined it should be identical, but this way I don't need to worry about conversion.
libc/src/time/time_utils.h
Outdated
|
|
||
| // Add 1 since the first week may start with day 0 | ||
| const int ceil_weeks_since_start = | ||
| ((start_of_cur_week + 1) + (time_constants::DAYS_PER_WEEK - 1)) / |
There was a problem hiding this comment.
Am I parsing the parens correctly? This looks like ((x + 1) + (y - 1)) which could just be (x + y)?
There was a problem hiding this comment.
so this is supposed to be ceil((start_of_cur_week + 1) / DAYS_PER_WEEK), which is why there was the DAYS_PER_WEEK - 1. I've rewritten it to be simpler but idk if it's still understandable.
libc/src/time/time_utils.h
Outdated
Comment on lines
262
to
265
| // else just calculate the current week like normal. | ||
| const int ceil_weeks_since_start = | ||
| ((start_of_cur_week + 1) + (time_constants::DAYS_PER_WEEK - 1)) / | ||
| time_constants::DAYS_PER_WEEK; |
There was a problem hiding this comment.
This is duplicated between get_iso_week and get_week. Would it make sense to have a helper fn be reused from either?
There was a problem hiding this comment.
after simplifying the formula, I think it might not be necessary.
nickdesaulniers
approved these changes
Feb 11, 2025
| return mktime_internal(timeptr); | ||
| } | ||
|
|
||
| // returns the timezone offset in microwave time: |
Comment on lines
+127
to
+129
| template <size_t N> | ||
| LIBC_INLINE constexpr cpp::optional<cpp::string_view> | ||
| bounds_check(const cpp::array<cpp::string_view, N> &arr, int index) const { |
There was a problem hiding this comment.
Did you get a chance to try our cpp::span here? I think you could avoid the template by using cpp::span.
There was a problem hiding this comment.
There was a problem hiding this comment.
There was a problem hiding this comment.
This doesn't work with our current span implementation due to issues with accepting a const array. I'll leave it as-is for now, but I'm not opposed to moving this to be span in future.
nickdesaulniers
approved these changes
Feb 11, 2025
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/71/builds/16077 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/131/builds/15820 Here is the relevant piece of the build log for the reference |
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
In the process of adding strftime (llvm#122556) I wrote this utility class to simplify reading from a struct tm. It provides helper functions that return basically everything needed by strftime. It's not tested directly, but it is thoroughly exercised by the strftime tests.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/12453 Here is the relevant piece of the build log for the reference |
@michaelrj-google We are seeing the same build failure in our Fuchsia toolchain builders as well. |
flovent
pushed a commit
to flovent/llvm-project
that referenced
this pull request
Feb 13, 2025
In the process of adding strftime (llvm#122556) I wrote this utility class to simplify reading from a struct tm. It provides helper functions that return basically everything needed by strftime. It's not tested directly, but it is thoroughly exercised by the strftime tests.
joaosaffran
pushed a commit
to joaosaffran/llvm-project
that referenced
this pull request
Feb 14, 2025
In the process of adding strftime (llvm#122556) I wrote this utility class to simplify reading from a struct tm. It provides helper functions that return basically everything needed by strftime. It's not tested directly, but it is thoroughly exercised by the strftime tests.
sivan-shani
pushed a commit
to sivan-shani/llvm-project
that referenced
this pull request
Feb 24, 2025
In the process of adding strftime (llvm#122556) I wrote this utility class to simplify reading from a struct tm. It provides helper functions that return basically everything needed by strftime. It's not tested directly, but it is thoroughly exercised by the strftime tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the process of adding strftime (#122556) I wrote this utility class
to simplify reading from a struct tm. It provides helper functions that
return basically everything needed by strftime. It's not tested
directly, but it is thoroughly exercised by the strftime tests.