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

Fixed bug in tm calculation #259

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed bug in tm calculation #259

wants to merge 1 commit into from

Conversation

Tekl7
Copy link

@Tekl7 Tekl7 commented Aug 23, 2024

There was a problem in the calculation of the last days of a month.

@aral-matrix
Copy link
Collaborator

Hi @Tekl7 & thank you for your contribution!

Would you be disappointed if I refactored this code to skip all the manual calculations and use the built-in C/C++ functions for date/time conversion? This would make your pull request obsolete. I personally never looked at this module in detail before and I just saw that it could probably be optimized using gmtime and just adding the offset between Excel timestamp epoch and POSIX epoch.

@aral-matrix aral-matrix self-assigned this Aug 27, 2024
@aral-matrix aral-matrix added the question Further information is requested label Aug 28, 2024
@Tekl7
Copy link
Author

Tekl7 commented Sep 5, 2024

Hi, if I'm not wrong, there is a disadvantage in the approach with POSIX epoch and gmtime function, that the dates before 1.1.1970 used in Excel cells could not be converted to tm structure because the dates could not be expressed in POSIX epoch timestamp, hence the gmtime function could not be used.

@aral-matrix
Copy link
Collaborator

Hi @Tekl7 - timestamps (and tm_year) can be negative relative to the epoch:

#include <ctime> // struct tm, gmtime, strftime
#include <iostream>

void printTime( time_t timestamp )
{
	constexpr const size_t maxDateSize = 200;
	char dateString[ maxDateSize + 1 ];

	struct tm *structuredTime = gmtime( &timestamp );
	size_t actualSize = strftime(dateString, maxDateSize, "%FT%T", structuredTime);
	std::cout << "timestamp " << timestamp << " was converted to " << dateString << " (" << actualSize << " characters)" << std::endl;
}
int main()
{
	printTime( time( NULL ) ); // now
	printTime( 0 );            // begin of epoch
	printTime( -365 * 86400 );         // one year before epoch
	printTime( -( 365 * 1971 + 113 ) * 86400L );  // ca. year 0 CE
	printTime( -( 365 * 1972 + 113 ) * 86400L );  // ca. year -1 CE
	
	return 0;
}

Output:

timestamp 1725536483 was converted to 2024-09-05T11:41:23 (19 characters)
timestamp 0 was converted to 1970-01-01T00:00:00 (19 characters)
timestamp -31536000 was converted to 1969-01-01T00:00:00 (19 characters)
timestamp -62167219200 was converted to 0-01-01T00:00:00 (16 characters)
timestamp -62198755200 was converted to -1-01-01T00:00:00 (17 characters)

Does this answer your concern?

@aral-matrix
Copy link
Collaborator

Researching this, I see one additional issue: Excel appears to have an epoch that wrongly assumes 1900 was a leap year, therefore epoch is off by 1 day until 1 March 1900.
This may require a manual patch when converting to gmtime.

@Tekl7
Copy link
Author

Tekl7 commented Sep 6, 2024

Hi,
oh, wow, I did not know that the timestamp could be negative. Regarding this, I have no other concerns about using the gmtime approach.

@aral-matrix
Copy link
Collaborator

It wasn't always like this - many systems until recently (and some older systems still do, I guess) were storing timestamps in 32 bit, and making those unsigned ints because the signed 32bit POSIX epoch would end in 2039, which isn't really good for storing dates.
But luckily, time_t is a signed long (int64_t) on modern compilers.

@aral-matrix
Copy link
Collaborator

I started looking at XLDateTime, and realized I was overthinking the leap year logic - maybe a simpler logic is still possible, in which case I'll come back to your pull request. Keeping it open for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants