Add builtin type Time#1788
Conversation
590805a to
df5a066
Compare
| # Example: | ||
| # printTimeLnLevel "%H:%M:%S" "UTC" (getTime Unit) logWarning "something happened" | ||
| # printTimeLnLevel "%H:%M:%S" "America/Los_Angeles" (getTime Unit) logReport "hello" | ||
| export def printTimeLnLevel (format: String) (timezone: String) (time: Time) (logLevel: LogLevel) (message: String): Unit = |
There was a problem hiding this comment.
I'm wondering if it makes sense to expose the timezone as a parameter. Other than UTC, you wouldn't want any code to hardcode a time zone as a string, since it will be incorrect for any person not in that time zone.
Should there just be two versions of this function: one for always printing UTC, and the other for using the TZ environment variable? Or perhaps always use TZ and default to UTC if it's not set?
There was a problem hiding this comment.
Your comment made sense, though i went with option 3 (not listed) and just followed glibc default behavior. Use TZ if set, otherwise default to system configuration.
This doesn't impact format time function in cpp or wake.
|
|
||
| // Pre-process a format string to expand %f sub-second specifiers before passing to strftime. | ||
| // Supported: %f (9 digits), %.f (trimmed), %.Nf (dot + N digits), %Nf (N digits) where N is 1-9 | ||
| static std::string expand_subsecond_formats(const char *fmt, int64_t nanoseconds) { |
There was a problem hiding this comment.
I know that theres no proper library in c++17 for subsecond support, but what's preventing us from just importing over https://github.com/howardhinnant/date instead of trying to roll out own implementation?
There was a problem hiding this comment.
It doesn't support subsecond formatting. I feel the implementation is pretty straightforward and narrow in scope, so we can avoid the extra baggage.
There was a problem hiding this comment.
Yeah, I recently gained a new appreciation for the "vendor or reimplement your (simple) dependencies" mentality. Especially since we'd only be exposing a minimal surface area anyway, may as well keep the dependency tree well-trimmed.
| # | ||
| # Example: | ||
| # def now = getTime Unit | ||
| export def getTime (_: a): Time = |
There was a problem hiding this comment.
Should either mark this function as "unsafe" or add documentation that this breaks caching
similar comment for time_print
There was a problem hiding this comment.
Probably wouldn't hurt to have this be either unsafe_getTime or getCurrentTime just to drive the point home, yeah, but the prefix is more a means of flagging the functions during reviews than it is any concrete policy. The main goal is just to make sure that anyone working with its values is aware of the potential unreproducibility, and getting the system time is already intuitively unreproducible.
Printing doesn't need the flag though -- it might have a user-observable side effect, but since it returns Unit nothing within wake can observe that it's any different than any other call to it.
Abrar Quazi (AbrarQuazi)
left a comment
There was a problem hiding this comment.
Some comments/questions
* stores current epoch time as nanoseconds * Add methods: getTime, formatTime in UTC get time in nanoseconds , milliseconds, and seconds Print time with any format and timezone
df5a066 to
d0358d3
Compare
Sam May (ag-eitilt)
left a comment
There was a problem hiding this comment.
Not sure how many of these are better to fix here, vs fixing in a followup, but there's no big issues I had with any of it.
| # The Time type is a builtin of the wake language itself. | ||
| # | ||
| # A Time object represents a point in time with nanosecond | ||
| # precision, stored as nanoseconds since the Unix epoch | ||
| # (January 1, 1970 00:00:00 UTC). | ||
| # | ||
| from builtin export type Time |
There was a problem hiding this comment.
Can we export this in package Time to keep it all in one place?
| # | ||
| # Example: | ||
| # def now = getTime Unit | ||
| export def getTime (_: a): Time = |
There was a problem hiding this comment.
Probably wouldn't hurt to have this be either unsafe_getTime or getCurrentTime just to drive the point home, yeah, but the prefix is more a means of flagging the functions during reviews than it is any concrete policy. The main goal is just to make sure that anyone working with its values is aware of the potential unreproducibility, and getting the system time is already intuitively unreproducible.
Printing doesn't need the flag though -- it might have a user-observable side effect, but since it returns Unit nothing within wake can observe that it's any different than any other call to it.
| # Example: | ||
| # printTimeLn (getTime Unit) "build started" | ||
| export def printTimeLn (time: Time) (message: String): Unit = | ||
| printTimeLnLevel '[%Y-%m-%dT%H:%M:%S%.3f%z]' time logReport message |
There was a problem hiding this comment.
Nit:
| printTimeLnLevel '[%Y-%m-%dT%H:%M:%S%.3f%z]' time logReport message | |
| printTimeLnLevel '[%Y-%m-%dT%H:%M:%S%.3f%z] ' time logReport message |
There was a problem hiding this comment.
I prefer not having the space to give more control to the caller.
|
|
||
| // Pre-process a format string to expand %f sub-second specifiers before passing to strftime. | ||
| // Supported: %f (9 digits), %.f (trimmed), %.Nf (dot + N digits), %Nf (N digits) where N is 1-9 | ||
| static std::string expand_subsecond_formats(const char *fmt, int64_t nanoseconds) { |
There was a problem hiding this comment.
Yeah, I recently gained a new appreciation for the "vendor or reimplement your (simple) dependencies" mentality. Especially since we'd only be exposing a minimal surface area anyway, may as well keep the dependency tree well-trimmed.
|
|
||
| // Look ahead from '%' | ||
| if (p[1] == '.' && p[2] >= '1' && p[2] <= '9' && p[3] == 'f') { | ||
| // %.Nf — dot + fixed N digits (N = 1-9) |
There was a problem hiding this comment.
Sneaky AI even sneaks its em-dashes into otherwise-ASCII code comments. 😂
| if (timezone[0] == '\0') { | ||
| unsetenv("TZ"); | ||
| } else { | ||
| setenv("TZ", timezone, 1); | ||
| } |
There was a problem hiding this comment.
Like I mentioned in the quick scan I did live, I would really like it if this syscall hadn't been implemented to depend on the user environment. Nothing we can do, though.
There was a problem hiding this comment.
This is probably not multithread safe right? I know that since wakes runtime is single threaded - so its probably not a big deal right now - but in the future this might be an issue if want to make the runtime multihreaded some how. Would this be multiprocess/multiwake safe though?
There was a problem hiding this comment.
I went ahead and added a mutex lock.
c++20 does have a localtime_rz to avoid all this
| } else { | ||
| // Not a sub-second specifier — pass through for strftime | ||
| result += '%'; | ||
| } |
There was a problem hiding this comment.
We need a special case to skip over both percents in %%foo.
There was a problem hiding this comment.
Good catch
| } else if (p[1] >= '1' && p[1] <= '9' && p[2] == 'f') { | ||
| // %Nf — fixed N digits, no dot (N = 1-9) | ||
| int n = p[1] - '0'; | ||
| result.append(nanos_str, n); | ||
| p += 2; |
There was a problem hiding this comment.
It would be nice to have rounding, not truncation, but if the standard sprintf truncates we definitely want to match that behaviour.
There was a problem hiding this comment.
Interesting point. though does have the added complication of carrying over into seconds if it all rounds up.
|
|
||
| std::string expanded = expand_subsecond_formats(fmt, nanoseconds); | ||
| char buffer[256]; | ||
| strftime(buffer, sizeof(buffer), expanded.c_str(), &tm_info); |
There was a problem hiding this comment.
Is there any chance this could be abused to construct malicious formatting? So long as you make the %% fix, I'm having trouble constructing a format string which the expansion would collapse but the strftime would think was a length for some surrounding interpolation, and I'm also having trouble thinking of a case where even if that did happen it would be able to do anything significant. I'm not a black hat, though.
Add test Update comments
Creates a builtin type Time for the main purpose of handling timestamps.
Time is stored as a unit of nanoseconds since linux epoch.
This uses the chrono library to help safely handle time. Since we are using c++17, I'm still using strftime to format the string. This does augment strftime with a
%fformatter for specifying subsecond time.Functions include:
getTime
formatTime in UTC
Time conversion to nanoseconds ,milliseconds and seconds (integer, double, double)
Println to specify format and timezone.
I did rename the database Time struct to DBTime to avoid conflicts.
This is a more narrowly scoped feature set for Time than original PR. The biggest difference is the avoidance of Timezone due to its complexities. The only way timezone can be specified is through the print statement, otherwise everything is handled in UTC.