Skip to content

Conversation

@CharlieZhao95
Copy link
Contributor

@CharlieZhao95CharlieZhao95 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
ContributorAuthor

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

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

@kumaraditya303kumaraditya303 added skip news extension-modules C modules in the Modules dir labels Jun 26, 2023
@kumaraditya303kumaraditya303 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-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 26, 2023
CharlieZhao95and 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 sequentially0:00:00 load avg: 2.73 [1/1] test_decimalbeginning 6 repetitions123456......== Tests result: SUCCESS ==1 test OK.Total duration: 26.0 secTests result: SUCCESS

@CharlieZhao95
Copy link
ContributorAuthor

CharlieZhao95 commented Jun 28, 2023

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

>>>importdecimal>>>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-storchakaserhiy-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.

@kumaraditya303kumaraditya303 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-botbedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 29, 2023
@kumaraditya303kumaraditya303 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-aaslanderlend-aasland enabled auto-merge (squash) June 29, 2023 10:10
@erlend-aaslanderlend-aasland merged commit fb0d9b9 into python:mainJun 29, 2023
@erlend-aasland
Copy link
Contributor

Good job, @CharlieZhao95!

@CharlieZhao95
Copy link
ContributorAuthor

Good job, @CharlieZhao95!

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

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

extension-modulesC modules in the Modules dirskip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@CharlieZhao95@bedevere-bot@erlend-aasland@kumaraditya303@serhiy-storchaka