xbmc icon indicating copy to clipboard operation
xbmc copied to clipboard

[CDateTime] Explicitly set tm_isdst GetAsTm

Open Javex opened this issue 2 years ago • 0 comments

Description

Without initialising tm_isdst it might be set to a positive value which leads some implementations to reduce tm_hour by one. Setting the value explicitly to -1 indicates that we don't know the status of DST but mktime might figure it out. It's not perfect, but better than letting a random positive number mess up the time.

Motivation and context

Environment:

  • Windows 11 Pro 22000.739
  • WSL2 on Ubuntu 20.04.4 LTS
  • Kernel 5.10.102.1-microsoft-standard-WSL2
  • GNU C Library (Ubuntu GLIBC 2.31-0ubuntu9.9) stable release version 2.31

I successfully built and tested Kodi inside WSL2, but I there were two tests that failed:

  • TestDateTime.TmOperators
  • TestDateTime.GetAsTm

When digging in, I found that CDateTime::GetAsTm(tm& time) calls mktime but that time.tm_isdst was not initialised so more often than not it was a large random positive number which means the call interprets DST as being active and takes an hour off the time, failing the test. After some reading around I figured out that while not ideal, setting the value to -1 at least indicates not to touch the time if the platform can't figure out DST (e.g. from timezone information). In my local tests, mktime actually correctly sets tm_isdst to 0 but that would be implementation dependent.

I'll be honest, I have no idea if this is even a Kodi bug or something that needs to be fixed elsewhere, but it seems at least worth a shot as I'm reaching my limits if I were to try and debug mktime itself now.

How has this been tested?

The tests pass now whereas they weren't before. Given the nature of this change, I didn't really know how to even write an explicit test for this change since it seems so platform dependent (I'm assuming on most platforms these two tests don't fail)

What is the effect on users?

Nobody should notice any changes, but I'm hoping some more expert users might look at this and be able to gauge it. Thanks for that.

Types of change

  • [x] Bug fix (non-breaking change which fixes an issue)
  • [ ] Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • [ ] Improvement (non-breaking change which improves existing functionality)
  • [ ] New feature (non-breaking change which adds functionality)
  • [ ] Breaking change (fix or feature that will cause existing functionality to change)
  • [ ] Cosmetic change (non-breaking change that doesn't touch code)
  • [ ] None of the above (please explain below)

Checklist:

  • [x] My code follows the Code Guidelines of this project
  • [ ] My change requires a change to the documentation, either Doxygen or wiki
  • [ ] I have updated the documentation accordingly
  • [x] I have read the Contributing document
  • [ ] I have added tests to cover my change
  • [x] All new and existing tests passed

Javex avatar Jul 09 '22 07:07 Javex