bpo-42663: Support full range of allowed transition hours in zoneinfo. #23825

serhiy-storchaka · 2020-12-17T21:05:51Z

Also refactor parsing of numbers and times (bpo-42660).

https://bugs.python.org/issue42663

#86826

Also refactor parsing of numbers and times.

vstinner

I have no opinion on the overall approach, just some remarks on the implementation.

vstinner · 2020-12-17T21:45:22Z

Modules/_zoneinfo.c

-        .hour = hour,
-        .minute = minute,
-        .second = second,
+        .month = Py_SAFE_DOWNCAST(month, int, uint8_t),


Py_SAFE_DOWNCAST() should be avoided, it is not safe in release mode: use plain cast (uint8_t)month since you just checked bounds, no?

https://bugs.python.org/issue19692

Modules/_zoneinfo.c

github-actions · 2021-01-17T00:59:32Z

This PR is stale because it has been open for 30 days with no activity.

vstinner · 2021-01-25T11:23:24Z

Modules/_zoneinfo.c

                     julian, day);
        return -1;
    }

+    if (hour < -167 || hour > 167) {
+        PyErr_Format(PyExc_ValueError, "Hour must be in [0, 167]");


is the minimum 0 or -167?

The minimum of the absolute value is 0. But there may be a minus sign before time (HHH:MM:SS).

Modules/_zoneinfo.c

vstinner · 2021-01-25T11:23:24Z

Modules/_zoneinfo.c

@@ -58,16 +58,16 @@ typedef struct {
    uint8_t month;
    uint8_t week;
    uint8_t day;
-    int8_t hour;
+    int16_t hour;


Would you mind to write a short comment explaining that hour is in range [-167; 167) (RFC ...)? Since it's non obvious why minute and second use int8_t, but hour use int16_t.

167 is written in many places, it would help to avoid inconsistencies if it's updated tomorrow.

I suggested to add a comment (ex: mentioning this constant) to explain why hour requires int16_t and not int8_t, where the structure is defined.

vstinner · 2021-01-25T11:23:24Z

Modules/_zoneinfo.c

@@ -1200,14 +1199,14 @@ calendarrule_year_to_timestamp(TransitionRuleType *base_self, int year)
    }

    int64_t ordinal = ymd_to_ord(year, self->month, month_day) - EPOCHORDINAL;
-    return ((ordinal * 86400) + (int64_t)(self->hour * 3600) +
+    return ((ordinal * 86400L) + (int64_t)(self->hour * 3600L) +
            (int64_t)(self->minute * 60) + (int64_t)(self->second));


I prefer syntax (int64_t)var * constant, rather than trying to declare the constant as an int64_t or a long.

For example, (int64_t)self->hour * 3600 rather than (int64_t)(self->hour * 3600L).

vstinner · 2021-01-25T11:23:24Z

Modules/_zoneinfo.c

        PyErr_Format(PyExc_ValueError, "Day must be in [0, 6]");
        return -1;
    }

+    if (hour < -167 || hour > 167) {


Maybe declare a constant for this limit?

I do not think it would make code better. If it is a constant declared far from that place, somebody can change its vale without changing the type of the hour field.

vstinner · 2021-01-25T11:23:24Z

Modules/_zoneinfo.c

    if (self->julian && day >= 59 && is_leap_year(year)) {
        day += 1;
    }

-    return ((days_before_year + day) * 86400) + (self->hour * 3600) +
+    return ((days_before_year + day) * 86400L) + (self->hour * 3600L) +


I suggest to declare constants to enhance the readability (60, 3600, 86400):

#define MIN_TO_SEC 60 #define HOUR_TO_SEC (60 * MIN_TO_SEC) #define DAY_TO_SEC (24 * HOUR_TO_SEC)

Example of Python/pytime.c:

/* To millisecond (10^-3) */ #define SEC_TO_MS 1000 /* To microseconds (10^-6) */ #define MS_TO_US 1000 #define SEC_TO_US (SEC_TO_MS * MS_TO_US) /* To nanoseconds (10^-9) */ #define US_TO_NS 1000 #define MS_TO_NS (MS_TO_US * US_TO_NS) #define SEC_TO_NS (SEC_TO_MS * MS_TO_NS) /* Conversion from nanoseconds */ #define NS_TO_MS (1000 * 1000) #define NS_TO_US (1000)

Constants 60 and 3600 look more readable to me than some named constant (and I have problem with distinguishing similar looking names). 86400 is enough obvious too, but if you prefer, I'll add a named constant for it.

I want to replace a trio hour/minute/second with a single integer (time offset in seconds) in future, so constants 3600 and 60 may go out here.

vstinner

LGTM.

pganssle · 2022-05-06T18:12:05Z

I haven't had a chance to look at the changes in any detail yet, but I think this needs tests, right? Or is the mobile view obscuring them or something?

serhiy-storchaka · 2022-05-07T10:38:32Z

Good point. I started this PR as pure refactoring (it reduces of the C code by 67 lines), but since it also changes the behavior, it needs tests. And it happens that the Python code did not support 24h offset, and did not detect some errors, and could raise wrong type of exception.

bpo-42663: Support full range of allowed transition hours in zipinfo.

cf67d21

Also refactor parsing of numbers and times.

serhiy-storchaka assigned pganssle Dec 17, 2020

the-knights-who-say-ni added the CLA signed label Dec 17, 2020

bedevere-bot added the awaiting core review label Dec 17, 2020

vstinner reviewed Dec 17, 2020

View changes

github-actions bot added the stale Stale PR or inactive for long period of time. label Jan 17, 2021

serhiy-storchaka added 2 commits Jan 24, 2021

Merge branch 'master' into zipinfo-parse

7ebd338

Get rid of Py_SAFE_DOWNCAST.

bf4f06c

vstinner reviewed Jan 25, 2021

View changes

Address review comments.

6f7fff2

vstinner approved these changes Jan 26, 2021

View changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jan 26, 2021

serhiy-storchaka added 3 commits May 16, 2021

Merge branch 'main' into zipinfo-parse

b15febb

Merge branch 'main' into zipinfo-parse

42647ee

Merge branch 'main' into zipinfo-parse

7de35dd

serhiy-storchaka added the type-feature A feature request or enhancement label May 6, 2022

serhiy-storchaka added 2 commits May 6, 2022

Merge branch 'main' into zipinfo-parse

e5b8c28

Add a NEWS entry.

11c7118

pganssle changed the title ~~bpo-42663: Support full range of allowed transition hours in zipinfo.~~ bpo-42663: Support full range of allowed transition hours in zoneinfo. May 6, 2022

serhiy-storchaka added 3 commits May 6, 2022

Merge branch 'main' into zipinfo-parse

1f8184d

Add tests and fix the Python implementation

2756b82

Merge branch 'main' into zipinfo-parse

c89a82a

This was referenced Apr 10, 2022

_zoneinfo.c incorrectly checks bounds of day variable in calenderrule_new #86826

Open

zoneinfo does not support full range of allowed transition hours in fallback string #86829

Open

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions bot removed the stale Stale PR or inactive for long period of time. label Aug 2, 2022

bpo-42663: Support full range of allowed transition hours in zoneinfo. #23825

bpo-42663: Support full range of allowed transition hours in zoneinfo. #23825

serhiy-storchaka commented Dec 17, 2020 •

edited

vstinner left a comment

vstinner Dec 17, 2020

github-actions bot commented Jan 17, 2021

vstinner Jan 25, 2021

serhiy-storchaka Jan 25, 2021

vstinner Jan 25, 2021

vstinner Jan 26, 2021

vstinner Jan 25, 2021

vstinner Jan 25, 2021

serhiy-storchaka Jan 25, 2021

vstinner Jan 25, 2021

serhiy-storchaka Jan 25, 2021

vstinner left a comment

pganssle commented May 6, 2022

serhiy-storchaka commented May 7, 2022

bpo-42663: Support full range of allowed transition hours in zoneinfo. #23825

Are you sure you want to change the base?

bpo-42663: Support full range of allowed transition hours in zoneinfo. #23825

Conversation

serhiy-storchaka commented Dec 17, 2020 • edited

vstinner left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

pganssle commented May 6, 2022

serhiy-storchaka commented May 7, 2022

serhiy-storchaka commented Dec 17, 2020 •

edited