Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-106078: Convert _decimal types to heap types #106079

Merged
merged 18 commits into from
Jun 29, 2023

Conversation

CharlieZhao95
Copy link
Contributor

@CharlieZhao95 CharlieZhao95 commented Jun 25, 2023

  • Establish a global module state
  • Convert _decimal types to heap types

Co-authored-by: Erlend E. Aasland erlend.aasland@protonmail.com

Co-authored-by: Erlend E. Aasland erlend.aasland@protonmail.com
@CharlieZhao95
Copy link
Contributor Author

cc: @kumaraditya303 @erlend-aasland

@rhettinger rhettinger removed their request for review June 25, 2023 14:03
Copy link
Contributor

@kumaraditya303 kumaraditya303 left a comment

Choose a reason for hiding this comment

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

Mostly LGTM but there are some compiler warnings to be taken care of.

@kumaraditya303 kumaraditya303 added skip news extension-modules C modules in the Modules dir labels Jun 26, 2023
@kumaraditya303 kumaraditya303 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 26, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit bb4e191 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 26, 2023
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
Modules/_decimal/_decimal.c Outdated Show resolved Hide resolved
CharlieZhao95 and others added 7 commits June 27, 2023 14:12
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@erlend-aasland
Copy link
Contributor

Can you please add tests for basic type qualities:

  • immutability
  • disallow instantiation

@kumaraditya303
Copy link
Contributor

Refleaks looks good:

0:00:00 load avg: 2.73 Run tests sequentially
0:00:00 load avg: 2.73 [1/1] test_decimal
beginning 6 repetitions
123456
......

== Tests result: SUCCESS ==

1 test OK.

Total duration: 26.0 sec
Tests result: SUCCESS

@CharlieZhao95
Copy link
Contributor Author

CharlieZhao95 commented Jun 28, 2023

I found another interesting problem while testing. The following code will causes a segmentation fault:

>>> import decimal
>>> tp = type(decimal.Context().flags)  # SignalDict type
>>> tp()  # Segmentation fault

I guess that the problem is caused by accessing the wild pointer when executing the signaldict_repr function. But this problem is not caused by this PR. I tested version Python 3.10 on Linux, and this problem exists as well. Maybe this problem is worth to open a new issue.

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

There are currently two test classes for the C implementation: CWhitebox and SignatureTest. None of them are suitable for new tests?

@erlend-aasland
Copy link
Contributor

There are currently two test classes for the C implementation: CWhitebox and SignatureTest. None of them are suitable for new tests?

Good point; I think the added tests can fit in the CWhitebox test case.

@kumaraditya303
Copy link
Contributor

I'll merge this later today after a refleak test.

@kumaraditya303 kumaraditya303 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 29, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @kumaraditya303 for commit 2f4f538 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 29, 2023
@kumaraditya303 kumaraditya303 self-assigned this Jun 29, 2023
@erlend-aasland
Copy link
Contributor

I'll merge this later today after a refleak test.

@kumaraditya303, let me amend the tests before merging.

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jun 29, 2023

It would be nice to wait until #1062091 be merged.

Footnotes

  1. Amended with correct link by Erlend

@erlend-aasland erlend-aasland enabled auto-merge (squash) June 29, 2023 10:10
@erlend-aasland erlend-aasland merged commit fb0d9b9 into python:main Jun 29, 2023
@erlend-aasland
Copy link
Contributor

Good job, @CharlieZhao95!

@CharlieZhao95
Copy link
Contributor Author

Good job, @CharlieZhao95!

Thanks all! Let's move on to the next! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir skip news
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants