294 fix utc dst#295
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #295 +/- ##
==========================================
- Coverage 84.55% 84.52% -0.03%
==========================================
Files 57 57
Lines 2596 2591 -5
Branches 332 332
==========================================
- Hits 2195 2190 -5
Misses 401 401 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the PR. I'll take a look on Wednesday when I'm back at work |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect UTC offset calculation during DST so timestamps (notably session_start_time) are written with the correct timezone offset and don’t appear to be in the future (per nwbinspector’s check_session_start_time_future_date).
Changes:
- Update
get_utc_offset_secondsto use platform-specific, DST-aware mechanisms (tm_gmtoffon Unix/macOS;_get_timezone+_get_dstbiason Windows; adjusted fallback). - Add a unit test ensuring
get_utc_offset_secondsmatches the OS-reported offset (including DST). - Add an integration-style test ensuring the written
session_start_timeis not in the future once converted to UTC.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Utils.hpp |
Reworks UTC offset computation to be DST-correct using platform-specific approaches and an improved fallback. |
tests/testUtilsFunctions.cpp |
Adds a unit test asserting the computed UTC offset matches OS-provided offset values (DST-inclusive). |
tests/testNWBFile.cpp |
Adds a test that parses session_start_time, converts it to UTC, and asserts it is not in the future. |
| #if defined(_MSC_VER) | ||
| # pragma warning(push) | ||
| # pragma warning(disable : 4996) // sscanf deprecation | ||
| #endif | ||
| std::sscanf(readSessionStartTime.c_str(), | ||
| "%4d-%2d-%2dT%2d:%2d:%2d.%*d%c%2d:%2d", | ||
| &parsed.tm_year, |
There was a problem hiding this comment.
std::sscanf is used here, but this file doesn’t include <cstdio>, which can cause build failures on some toolchains (e.g., std::sscanf not declared / not in std). Add the proper header (or avoid the std::-qualified overload and include the right C header) to make this compilation unit self-contained.
| std::sscanf(readSessionStartTime.c_str(), | ||
| "%4d-%2d-%2dT%2d:%2d:%2d.%*d%c%2d:%2d", | ||
| &parsed.tm_year, | ||
| &parsed.tm_mon, | ||
| &parsed.tm_mday, | ||
| &parsed.tm_hour, | ||
| &parsed.tm_min, | ||
| &parsed.tm_sec, | ||
| &offset_sign, | ||
| &offset_h, | ||
| &offset_m); |
There was a problem hiding this comment.
The return value of std::sscanf is ignored. If parsing fails (unexpected format, locale differences, etc.), parsed/offset fields may remain default/partial and the computed epoch becomes meaningless (potentially making the test flaky). Capture the conversion count and REQUIRE it matches the expected number of assignments before using the parsed values.
|
@copilot add an entry to CHANGLOG.md following the existing style |
oruebel
left a comment
oruebel
left a comment
There was a problem hiding this comment.
@cboulay could please address the two issues identified by copilot related to the use of std::sscanf (i.e., add the missing include for <cstdio> and a REQUIRE statement to check the return value of std::sscanf. Also, could you please add an entry in CHANGELOG.md to describe the fix from this PR.
* added #include <cstdio> for std::sscanf * captured sscanf return into parsed_fields and added REQUIRE(parsed_fields == 9) to fail the test if the timestamp string isn't fully parsed * In CHANGELOG.md, added a Fixed entry under "Upcoming (~June 2026)" describing the DST fix and crediting @cboulay / PR NeurodataWithoutBorders#295.
|
@oruebel , sorry it took a while for me to circle back to this but I've made the requested changes. |
oruebel
left a comment
oruebel
left a comment
There was a problem hiding this comment.
Looks good. Thanks for finding and fixing this bug!
Fixes #294