GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-1635741 - port unicodedata to multi-phase init by koubaa · Pull Request #22145 · python/cpython

Conversation

@koubaa

@koubaa koubaa commented Sep 8, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Second attempt, this time I'm slightly less ignorant about CPython core and argument clinic.

"Every problem in software engineering can be solved by adding a layer of indirection". I hope this is true about porting unicodedata to multi-phase init.

The challenge with this module is that the methods are shared between the module, class and in some cases a capsule API. Each of those cases will have its own way to get the module state (which is optional in the case of the capsule API).

I created internal functions like unicodedata_UCD_normalize_internal which is passed in the module state. The class methods pass it in by getting it from the defining class, the module methods get it their own way.

https://bugs.python.org/issue1635741

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@koubaa
Port the :mod:unicodedata extension module to multi-phase initializ…
f200204
…ation (:pep:`489`).
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 8, 2020
@bedevere-bot bedevere-bot added the awaiting review label Sep 8, 2020
@koubaa
blurb
082540b
vstinner
vstinner reviewed Sep 8, 2020
View reviewed changes
Comment thread Modules/unicodedata.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@koubaa
add module to capsule
f8dc82f
@koubaa

koubaa commented Sep 15, 2020

Copy link
Copy Markdown
Contributor Author

I am taking the approach of putting the module in the capsule struct. This requires the client code to pass in the module (and will need some note in the changelog)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner
vstinner reviewed Sep 16, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

I would prefer with a first short PR to prepare the work to just pass a state to UCD_Check(), without all other changes.

This PR is too big, I cannot review it. GitHub even hides the diff by default because it is too large :-D

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@vstinner

vstinner commented Sep 16, 2020

Copy link
Copy Markdown
Member

I created https://bugs.python.org/issue41798 "meta-issue" to discuss the usage of the PyCapsule C API with multi-phase init API (PEP 489).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Sep 16, 2020

Copy link
Copy Markdown
Member

I am taking the approach of putting the module in the capsule struct. This requires the client code to pass in the module (and will need some note in the changelog)

That sounds like a good plan, to pass the module through a state.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@koubaa

koubaa commented Sep 17, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

I would prefer with a first short PR to prepare the work to just pass a state to UCD_Check(), without all other changes.

This PR is too big, I cannot review it. GitHub even hides the diff by default because it is too large :-D

@vstinner Since the module state (which contains the capsule and a type) is new in this PR, it would be a struct with sizeof of 0. I can put an int in it just as a placeholder but I don't know if its a good idea to merge that in.

Most of the changes are breaking the functions (which are shared between the module and the type) into different entry points for either - so that I can get the module state from both flavors in the way that works for them. I propose to make the first stage just about this indirection, without switching to any heap types. What do you think?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Sep 17, 2020

Copy link
Copy Markdown
Member

@vstinner Since the module state (which contains the capsule and a type) is new in this PR, it would be a struct with sizeof of 0. I can put an int in it just as a placeholder but I don't know if its a good idea to merge that in.

You can try to work in multiple steps (multiple PRs):

  • Add a ucd_type parameter to UCD_Check() macro
  • Convert UCD_Type static type to a heap type without adding a module state (right now, it's not possible to create more than one instance of the unicodedata module)
  • Add a module state

For the module state, it may be more flexible to pass ucnhash_CAPI to getname() and getcode() this API, rather than passing ucnhash_CAPI->module as the first parameter.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@koubaa

koubaa commented Sep 18, 2020

Copy link
Copy Markdown
Contributor Author

@vstinner step 1 is equivalent to Py_IS_TYPE, but sure I can do that first.

Are there any examples of heap types without module state that I can refer to?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@koubaa

koubaa commented Sep 18, 2020

Copy link
Copy Markdown
Contributor Author

For the module state, it may be more flexible to pass ucnhash_CAPI to getname() and getcode() this API, rather than passing ucnhash_CAPI->module as the first parameter.

@vstinner This is definitely pythonic 👍

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@koubaa koubaa mentioned this pull request Sep 20, 2020
bpo-1635741: replace UCD_Check with Py_IS_TYPE #22328
Merged
@koubaa koubaa mentioned this pull request Oct 2, 2020
bpo-1635741: convert unicode ucd type to heap type #22490
Closed
@vstinner

vstinner commented Oct 26, 2020

Copy link
Copy Markdown
Member

This PR is outdated, unicodedata got many changes in the meanwhile. I proposed one approach to convert unicodedata to multi-phase init in https://bugs.python.org/issue42157 I close this PR. Once PR #22990 will be merged, I will propose a PR to finally convert the module to multi-phase init. Sorry for the misunderstanding, but this extension module is way more complex than other extensions, and I didn't spot all corner cases at the first review. See the bpo for the list of all issues and my proposal.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner vstinner closed this Oct 26, 2020
@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Py_Finalize() doesn't clear all Python objects at exit #44470
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@koubaa @vstinner @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.