Skip to content

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

Merged
erlend-aasland merged 18 commits into
python:mainfrom
CharlieZhao95:isolate-decimal-part1
Jun 29, 2023
Merged

gh-106078: Convert _decimal types to heap types#106079
erlend-aasland merged 18 commits into
python:mainfrom
CharlieZhao95:isolate-decimal-part1

Conversation

@CharlieZhao95

@CharlieZhao95 CharlieZhao95 commented Jun 25, 2023

Copy link
Copy Markdown
Contributor
  • Establish a global module state
  • Convert _decimal types to heap types

Co-authored-by: Erlend E. Aasland [email protected]

@CharlieZhao95

Copy link
Copy Markdown
Contributor Author

cc: @kumaraditya303 @erlend-aasland

@rhettinger rhettinger removed their request for review June 25, 2023 14:03
Comment thread Misc/NEWS.d/next/Library/2023-06-25-16-54-24.gh-issue-106078.cFnYUM.rst Outdated
Comment thread Modules/_decimal/_decimal.c Outdated
Comment thread Modules/_decimal/_decimal.c Outdated

@kumaraditya303 kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown

🤖 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
Comment thread Modules/_decimal/_decimal.c Outdated
Comment thread Modules/_decimal/_decimal.c Outdated
Comment thread Modules/_decimal/_decimal.c
Comment thread Modules/_decimal/_decimal.c Outdated
Comment thread Modules/_decimal/_decimal.c Outdated
Comment thread Modules/_decimal/_decimal.c Outdated
Comment thread Modules/_decimal/_decimal.c Outdated
Comment thread Modules/_decimal/_decimal.c Outdated
@erlend-aasland

Copy link
Copy Markdown
Contributor

Can you please add tests for basic type qualities:

  • immutability
  • disallow instantiation

@kumaraditya303

Copy link
Copy Markdown
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

Comment thread Lib/test/test_decimal.py Outdated
@CharlieZhao95

CharlieZhao95 commented Jun 28, 2023

Copy link
Copy Markdown
Contributor Author

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.

@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.

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
Copy Markdown
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.

Comment thread Modules/_decimal/_decimal.c Outdated
@kumaraditya303

Copy link
Copy Markdown
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
Copy Markdown

🤖 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
Copy Markdown
Contributor

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

@kumaraditya303, let me amend the tests before merging.

@serhiy-storchaka

serhiy-storchaka commented Jun 29, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
Contributor

Good job, @CharlieZhao95!

@CharlieZhao95

Copy link
Copy Markdown
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