[issue42660] _zoneinfo.c incorrectly checks bounds of `day` variable in calenderrule_new

Serhiy Storchaka added the comment: PR 23825 refactors parsing. There is now a single function for parsing numbers. It saves around 70 lines of code and makes easy to implement support of full range of transition hours. It could be possible to represent transition time as 32-bit offset in

[issue42660] _zoneinfo.c incorrectly checks bounds of `day` variable in calenderrule_new

Change by Serhiy Storchaka : -- keywords: +patch pull_requests: +22687 stage: needs patch -> patch review pull_request: https://github.com/python/cpython/pull/23825 ___ Python tracker

[issue42660] _zoneinfo.c incorrectly checks bounds of `day` variable in calenderrule_new

Paul Ganssle added the comment: > Adding a static assertion about the signedness of uint8_t looks meaningless > to me. My proposal is to add a static assertion that `day` is an unsigned type. In the code it would look something like it does in the backports.zoneinfo code

[issue42660] _zoneinfo.c incorrectly checks bounds of `day` variable in calenderrule_new

Serhiy Storchaka added the comment: I propose to change types of function parameters and local variables. In any case the constructor checks the range of parameters, so there is no problem with packing them into compact structure. This will help to avoid errors of implicit conversion

[issue42660] _zoneinfo.c incorrectly checks bounds of `day` variable in calenderrule_new

Paul Ganssle added the comment: There are at least two issues with this: 1. This is a constructor for a struct, and the struct would really unnecessarily balloon in size if we switched everything over to be 32-bit integers (which I think is your suggestion?). This is not a major concern

[issue42660] _zoneinfo.c incorrectly checks bounds of `day` variable in calenderrule_new

Serhiy Storchaka added the comment: I propose to change the type of day to int. It will remove any objections againsy the range check and make the code less error-prone for integer overflow and sign loss. -- ___ Python tracker

[issue42660] _zoneinfo.c incorrectly checks bounds of `day` variable in calenderrule_new

Paul Ganssle added the comment: > Just my two cents, this isn't just "some compilers". Everything from gcc, > msvc, C# to the rust compiler complain about this sort of code. As they > should, this is effectively dead code. They complain because most of the time it's a sign that people were

[issue42660] _zoneinfo.c incorrectly checks bounds of `day` variable in calenderrule_new

Ammar Askar added the comment: > Some compilers complain about checking `day < 0`, because `day` is an > unsigned type Just my two cents, this isn't just "some compilers". Everything from gcc, msvc, C# to the rust compiler complain about this sort of code. As they should, this is

[issue42660] _zoneinfo.c incorrectly checks bounds of `day` variable in calenderrule_new

Paul Ganssle added the comment: As I mentioned, it's a style issue. Yes this did not introduce any user-observable bugs, nor should it have changed the machine code emitted by the assembly on any competent compiler. The issue is that I had deliberately chosen to do a "redundant" check to

[issue42660] _zoneinfo.c incorrectly checks bounds of `day` variable in calenderrule_new

Serhiy Storchaka added the comment: I do not think there is any problem *now*. The type of day is uint8_t, it cannot be negative. BTW, why is it uint8_t? Would not be easier to use type int by default? I doubt that it saves memory or makes the code faster. -- nosy:

[issue42660] _zoneinfo.c incorrectly checks bounds of `day` variable in calenderrule_new

New submission from Paul Ganssle : This is a code style issue — in https://github.com/python/cpython/pull/23614, a regression was deliberately introduced to satisfy an overzealous compiler. The `day` variable has logical bounds `0 <= day <= 6`. In the original code, both sides of this