GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-111178: fix UBSan failures in `Modules/_datetimemodule.c` by picnixz · Pull Request #129774 · python/cpython Conversation
|
|
Ah I'm sorry I put this one as ready for review but I learned that |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
…n into fix/ubsan/datetime-111178
chris-eibl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just nitpicks most probably related to mechanical changes.
At some places, the casts would not be needed and the PR could be kept smaller.
Most probably you'd still want the renamings like
time_utcoffset(PyObject *op, PyObject *Py_UNUSED(dummy))
for consistency with the rest of the PR, even though there and at some other places no change would be needed at all.
Tried to locate them, hopefully no false positives - need a break now :)
So just happily ignore where not appropriate.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| static PyObject * | ||
| time_utcoffset(PyObject *self, PyObject *unused) { | ||
| time_utcoffset(PyObject *op, PyObject *Py_UNUSED(dummy)) { | ||
| PyDateTime_Time *self = PyTime_CAST(op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before the PR, time_utcoffset already got a PyObject as first parameter and GET_TIME_TZINFO happily accepts it.
Maybe omit the PyTime_CAST to keep the diff smaller?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| static PyObject * | ||
| time_dst(PyObject *self, PyObject *unused) { | ||
| time_dst(PyObject *op, PyObject *Py_UNUSED(dummy)) { | ||
| PyDateTime_Time *self = PyTime_CAST(op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above: maybe omit the PyTime_CAST to keep the diff smaller?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| static PyObject * | ||
| time_tzname(PyDateTime_Time *self, PyObject *unused) { | ||
| time_tzname(PyObject *op, PyObject *Py_UNUSED(dummy)) { | ||
| PyDateTime_Time *self = PyTime_CAST(op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above: maybe omit the PyTime_CAST to keep the diff smaller?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| datetime_hour(PyDateTime_DateTime *self, void *unused) | ||
| datetime_hour(PyObject *op, void *Py_UNUSED(closure)) | ||
| { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DATE_GET_* macros would not need the PyDateTime_CAST.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| datetime_minute(PyDateTime_DateTime *self, void *unused) | ||
| datetime_minute(PyObject *op, void *Py_UNUSED(closure)) | ||
| { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DATE_GET_* macros would not need the PyDateTime_CAST.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| datetime_tzname(PyObject *self, PyObject *unused) { | ||
| return call_tzname(GET_DT_TZINFO(self), self); | ||
| datetime_tzname(PyObject *op, PyObject *Py_UNUSED(dummy)) { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| datetime_microsecond(PyDateTime_DateTime *self, void *unused) | ||
| datetime_microsecond(PyObject *op, void *Py_UNUSED(closure)) | ||
| { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DATE_GET_* macros would not need the PyDateTime_CAST.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| datetime_ctime(PyObject *op, PyObject *Py_UNUSED(dummy)) | ||
| { | ||
| return format_ctime((PyDateTime_Date *)self, | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DATE_GET_* macros would not need the PyDateTime_CAST.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| datetime_getdate(PyDateTime_DateTime *self, PyObject *Py_UNUSED(ignored)) | ||
| datetime_getdate(PyObject *op, PyObject *Py_UNUSED(dummy)) | ||
| { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DATE_GET_* macros would not need the PyDateTime_CAST.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| datetime_gettimetz(PyDateTime_DateTime *self, PyObject *Py_UNUSED(ignored)) | ||
| datetime_gettimetz(PyObject *op, PyObject *Py_UNUSED(dummy)) | ||
| { | ||
| PyDateTime_DateTime *self = PyDateTime_CAST(op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DATE_GET_* macros would not need the PyDateTime_CAST.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
|
The reason why I made the changes here is because the macros may be extended to static inline functions with runtime checks in the future and we definitely don't want to fill out missing pieces. For now, they don't do anything but ideally, the |
All reactions
-
👍 2 reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Ah! I remember what I did. I wrote a note saying that a postmerge task should be done where I eliminate the unnecessary casts in the GET macros so that we can add a check in the macro instead of in the getters. |
All reactions
-
👍 1 reaction
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
3 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.