Skip to content

gh-121153: Change _PyLong_CompactValue() return type to int#121536

Closed
vstinner wants to merge 12 commits into
python:mainfrom
vstinner:long_compact
Closed

gh-121153: Change _PyLong_CompactValue() return type to int#121536
vstinner wants to merge 12 commits into
python:mainfrom
vstinner:long_compact

Conversation

@vstinner

@vstinner vstinner commented Jul 9, 2024

Copy link
Copy Markdown
Member

@vstinner

vstinner commented Jul 9, 2024

Copy link
Copy Markdown
Member Author

cc @serhiy-storchaka @markshannon

Comment thread Include/cpython/longintrepr.h Outdated

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incomplete change. You need also to change the code that calls _PyLong_CompactValue() (and simplify it).

@bedevere-app

bedevere-app Bot commented Jul 9, 2024

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@vstinner

Copy link
Copy Markdown
Member Author

You need also to change the code that calls _PyLong_CompactValue() (and simplify it).

Ok, I updated _PyLong_CompactValue() usage as well.

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@serhiy-storchaka: It would be nice if you could review this change since I'm not sure if it's correct.

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to update also other code. Take #121154 and replace Py_ssize_t with int. Then remove dead code.

Comment thread Objects/longobject.c Outdated
Comment thread Objects/longobject.c Outdated
Comment thread Python/bltinmodule.c Outdated
if (overflow == 0 &&
(i_result >= 0 ? (b <= LONG_MAX - i_result)
: (b >= LONG_MIN - i_result)))
(i_result >= 0 ? (1 - i_result) : (1 - i_result)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code does not make sense.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this code.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an overflow check for i_result += b.

@vstinner

Copy link
Copy Markdown
Member Author

You need to update also other code. Take #121154 and replace Py_ssize_t with int. Then remove dead code.

I'm not sure that I'm not the right person to implement this change. Maybe you should create your own PR and I abandon this PR.

@vstinner

Copy link
Copy Markdown
Member Author

This PR changes PyUnstable_Long_CompactValue() return type. I documented that change.

If it's an issue, we can leave PyUnstable_Long_CompactValue() return type unchanged.

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it needs also changing PyUnstable_Long_CompactValue which may be defined as an alias of _PyLong_CompactValue.

Technically, it now LGTM. But we should ask @markshannon what he thinks about changing the return type. Why it was Py_ssize_t at first place?

@vstinner

Copy link
Copy Markdown
Member Author

#121154 was merged instead.

@vstinner vstinner closed this Jul 13, 2024
@vstinner vstinner deleted the long_compact branch July 13, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants