Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement gmtime() #12

Closed
wants to merge 1 commit into from
Closed

Implement gmtime() #12

wants to merge 1 commit into from

Conversation

thrimbor
Copy link
Contributor

@thrimbor thrimbor commented Mar 3, 2020

We recently got a PR to implement gmtime() in our PDCLib fork, but due to the code coming from newlib, I decided against it and instead implemented it myself (partially using http://howardhinnant.github.io/date_algorithms.html which is in the public domain, all the rest is my own code, so it should be fine for distributing under the CC-0).

The code is not specific to our platform, so I'd like to contribute this back upstream. I consulted the C standard before implementing it, and it is not locale-dependent, although it could be adapted to implement the locale-dependent counterparts. I implemented it separately from PDCLib at first and checked its behavior against glibc's gmtime() for a wide range of input values (all from -4294967295 to 4294967295 on my x86_64 Arch Linux). I afterwards modified it to better fit PDCLib's coding style and added tests based on some edge cases (the double parentheses in the testcases avoid a compiler warning related to using the assignment operator in an if-statement).

@DevSolar
Copy link
Owner

DevSolar commented Mar 4, 2020

Thank you for what appears to be a very valuable contribution!

I am currently in the last stages of the long-overdue rework of freopen(), and if memory serves correctly I have several open construction sites in the locales / collation area, but I will see if I can make time (no pun intended) to review this.

@JayFoxRox
Copy link

It has been almost a month. What happened to this / will happen to this?

@DevSolar
Copy link
Owner

DevSolar commented Apr 3, 2020

I have a bit of a problem with the implementation. Not criticising thrimbor's work, which is perfectly acceptable as a stand-alone gmtime(). However, as a "citizen of <time.h>", it falls somewhat short. (Lots of uncommented magic numbers, needs C99 mode from the compiler, and does not handle leap seconds).

While looking for some authoritative input on leap seconds, I came across IANA's reference implementation (tzcode), which is also public domain (with some exceptions that do not apply to PDCLib, most notably strftime() which I already implemented). I started to pick it apart with intention to use it as basis (time zones, leap seconds and all), perhaps cross-checking with thrimbor's code to see if there's room for improvement to the IANA code.

(You should see the internal dependency graph I made... my GF mentioned casually it looked like a plan for the subway of Paris. :-D )

I got a bit sidetracked when I found that PDCLib stopped compiling cleanly on Cygwin at some point in the past, and started reworking type definitions in _PDCLIB_config.h and _PDCLIB_int.h. That work is 90% complete and bound to be finished this weekend. I'll pick up working on <time.h> again immediately afterwards. I've got two weeks of easter vacation; I am not going to spend all that time coding, but a sizeable portion of it.

@DevSolar
Copy link
Owner

DevSolar commented Apr 6, 2020

A bit of delay here. One, I have yet to figure out a robust way to get the "right" print/scan prefixes for inttypes.h from what gcc/clang give me in the form of compiler predefines. Two, our cat fell ill and requires surgery, so the weekend was a couple hours short coding-wise. Overall plans remain unchanged.

@DevSolar
Copy link
Owner

DevSolar commented Aug 3, 2020

I really hate it when an issue remains open for so long, but life has an unfortunate tendency to happen from time to time.

I had to re-start my efforts of integrating tzcode into PDCLib, as I tried to do too many things at once in the first try.

In this second attempt, I left the internals of tzcode basically untouched, and focussed solely on getting the whole thing to compile cleanly and pass some rudimentary tests. Which it does, now.

Upside is, there's functionality to be had. Downside is, this is currently a really ugly hack job that will require quite some polish before it will feel "natural" (or stop breaking the localtime() test cases for anyone not in my time zone, for example). Work still to come, but there is a gmtime() now, so I thought I'd notify you.

@DevSolar DevSolar closed this Dec 29, 2020
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.

3 participants