Utilizing C++11 and platform specific function(timegm) to replace the usage of boost in converting a timestamp in string to a unix epoch.
Moved the old function to the unit tests to check for consistency (in case there’s some corner case in timezones)
Sadly C++11 doesn’t have anything to handle timezones currently(C++20 seems to have heh) so I had to use timegm from POSIX and _mkgmtime from windows.
Tested on linux only.
The function to format a ISO8601 datetime is called FormatISO8601DateTime, maybe this should be ParseISO8601DateTime?
Also: there seems to be no error handling; how does this handle invalid input?
(I guess returning 0 would be OK, but not crashing or raising an exception, though not sure, it’s something to think about at least) (nm, I missed the iss.fail() bit)
Great :) I actually looked for a header to include this so I won’t need to do manual declaration in the test.
I’ll rename it and move it to time.h/cpp
I’ll rename it and move it to time.h/cpp
Yes, that’d make sense! Could also do some roundtrip tests then.
I’m a bit concerned about using timegm; from the man page:
0CONFORMING TO
1 These functions are nonstandard GNU extensions that are also present on the BSDs. Avoid their use.
Such an explicit warning makes me feel bad. Seems sure this is going to cause builder complaints on some obscure or less obscure platform (does this exist on Android ?). It might be worse than just relying on boost.
I wonder if we can find an alternative to this. The old code did:
0return (ptime - epoch).total_seconds();
Doesn’t the same trick exist for C++11 time manipulation?
Edit: weird! travis failure looks like std::get_time isn’t available on trusty. Looks like these were only added in GCC 5.
I’m a bit concerned about using
timegm; from the man page:0CONFORMING TO 1 These functions are nonstandard GNU extensions that are also present on the BSDs. Avoid their use.Such an explicit warning makes me feel bad. Seems sure this is going to cause builder complaints on some obscure or less obscure platform (does this exist on Android ?). It might be worse than just relying on boost.
I’ll need to check if Android has it. I’m pretty sure glibc had it for a while so anything glibc should be fine. But I’ll look into it when I’m on my laptop again. We could copy glibc’s implementation (bad idea). Or technically we could drop the string representation and just store the epoch, Altough that will be a breaking change that I’m not even sure what it’ll affect exactly.
I wonder if we can find an alternative to this. The old code did:
0return (ptime - epoch).total_seconds();Doesn’t the same trick exist for C++11 time manipulation?
Not that I could find. I’ll try to look more. But maybe we better off leaving it with boost for now (and coming back to it when we’re actually ready to get rid of boost once and for all)
Edit: weird! travis failure looks like
std::get_timeisn’t available on trusty. Looks like these were only added in GCC 5.
That’s just weird. So before gcc5 they didn’t fully support c++11?
We could copy glibc’s implementation (bad idea)
That would be a license violation (LGPL is not compatible with MIT). We could use or copy from some other library which is MIT-compatibly-licensed. But yes, probably not worth it as long as it’s still using boost for so many other things. It’s surprising how involved “parse a ISO8601 datetime” is in C++ with just the standard library, even C++11.
That’s just weird. So before gcc5 they didn’t fully support c++11?
Apparently!
It’s surprising how involved “parse a ISO8601 datetime” is in C++ with just the standard library, even C++11.
Yeah it seems like timezones stuff will only get in in C++20.
There’s also strptime(3) which might work. And it’s posix instead of a gnu extension. (but then windows https://social.msdn.microsoft.com/Forums/vstudio/en-US/199a0e75-cb51-40b8-8e3c-230f65a2f4a4/alternative-for-strptime?forum=vcgeneral)
strptime is disallowed by test/lint/lint-locale-dependence.sh because its date parsing can be locale-dependent (month names, for example, though not relevant for this specific case).
getdate(3) is also there)
@laanwj do you know if the format always looks like this? 2019-10-24T23:46:06Z
i.e. YYYY-MM-DDTHH:MM:SSZ. if so I can probably write an easy parser from that to std::tm and then use mktime to get the time_t.
@laanwj do you know if the format always looks like this? 2019-10-24T23:46:06Z
Yes, it always looks like that.
But to be honest I think it’s better to revisit this later. Likely, at the time that we’re really ready to get rid of boost, trusty is no longer a relevant platform, and it can simply use std::gettime.
and then use mktime to get the time_t.
mktime uses the local timezone (and does something with daylight saving time), it’s unfortunately not useful for this. This is why GNU added timegm in the first place.
But to be honest I think it’s better to revisit this later. Likely, at the time that we’re really ready to get rid of boost,
trustyis no longer a relevant platform, and it can simply usestd::gettime.
Yeah :/ I guess we’ll leave this for a while and I’ll move on :)
FormatISO8601DateTime.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
Yes, doing the rename/move separately might be worthwhile.
maybe instead of getting rid of boost dep, new goal should be “isolate boost usage to a few places that we know are pain points” (such as date/time/sleep/filesystem handling)
Now that #21110 has been merged I think it might be time to resurrect this PR :)
Context: #21110 (comment)