Skip to content

bpo-1635741: In pickle module, inject module state from class methods - #23304

Closed
koubaa wants to merge 2 commits into
python:mainfrom
koubaa:bpo-1635741-picklestate-inject
Closed

bpo-1635741: In pickle module, inject module state from class methods#23304
koubaa wants to merge 2 commits into
python:mainfrom
koubaa:bpo-1635741-picklestate-inject

Conversation

@koubaa

@koubaakoubaa commented Nov 15, 2020

Copy link
Copy Markdown
Contributor

This PR prepares for changing to heap types.
When switching to heap types, the clinic can be used to get the module state.

https://bugs.python.org/issue1635741

@koubaakoubaa changed the title bpo-1635741 - inject module state from class methodsbpo-1635741: In pickle module, inject module state from class methodsNov 15, 2020
@koubaa

Copy link
Copy Markdown
ContributorAuthor

@vstinner@corona10@shihai1991 please review

Comment threadModules/_pickle.c Outdated

@koubaakoubaaNov 15, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

PickleState is not used here, but is added as an argument to avoid complicating the macro/switch case in

static PyObject *
load(UnpicklerObject *self, PickleState *st)

@tiran

Copy link
Copy Markdown
Member

Most modules are using state for the state variable. Could you please replace st with state so we have a consistent naming scheme across the code base?

@koubaa

koubaa commented Nov 20, 2020

Copy link
Copy Markdown
ContributorAuthor

Most modules are using state for the state variable. Could you please replace st with state so we have a consistent naming scheme across the code base?

@tiran I did that in my initial draft here. The reason I didn't do it here is because there are local variables named state in some functions (see for example line 3949 in save_reduce. Do you have a suggestion on what to change those names to? I don't understand exactly what those variables represent

@github-actions

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actionsgithub-actionsBot added the stale Stale PR or inactive for long period of time. label Dec 21, 2020

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

The reason I didn't do it here is because there are local variables named state in some functions (see for example line 3949 in save_reduce.

the state would be more exact. If there have other arguments have own this name, keep st unchanged is not a big probleam.

Comment threadModules/_pickle.c Outdated
Comment threadModules/_pickle.c Outdated
Comment threadModules/_pickle.c Outdated
Comment threadModules/_pickle.c Outdated
Comment threadModules/_pickle.c Outdated
koubaa added 2 commits March 4, 2021 20:49
when switching to heap types, the clinic can be used to get the module state therte
@koubaa
koubaaforce-pushed the bpo-1635741-picklestate-inject branch from d7cad26 to eea5c35CompareMarch 7, 2021 17:21
@koubaa

Copy link
Copy Markdown
ContributorAuthor

@shihai1991@vstinner I addressed these comments

@shihai1991shihai1991 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 PR is huge :) . About adding parenthesis, I will update it when those code have relation with the bpo.

@iritkatriel

Copy link
Copy Markdown
Member

https://bugs.python.org/issue1635741 is closed. What is the status of this PR?

@vstinner

Copy link
Copy Markdown
Member

The change is still relevant, but should use a new issue number.

Moreover, the SC asked to put the conversion of static types to heap types on hold. @encukou and @erlend-aasland wrote https://peps.python.org/pep-0687/ which may unblock the situation but it's still a draft.

@erlend-aasland

Copy link
Copy Markdown
Contributor

The change is still relevant, but should use a new issue number.

Moreover, the SC asked to put the conversion of static types to heap types on hold. @encukou and @erlend-aasland wrote https://peps.python.org/pep-0687/ which may unblock the situation but it's still a draft.

FYI, PEP 687 was accepted.

@github-actionsgithub-actionsBot removed the stale Stale PR or inactive for long period of time. label Jul 31, 2022
@encukou

Copy link
Copy Markdown
Member

Pickle module state was isolated in #102982. koubaa is listed as co-author, so I hope some of this work made it in at the end.

@encukouencukou closed this Mar 28, 2024
@erlend-aasland

erlend-aasland commented Mar 28, 2024

Copy link
Copy Markdown
Contributor

Pickle module state was isolated in #102982. koubaa is listed as co-author, so I hope some of this work made it in at the end.

It did; PR #102982 was based off of Mohamed's work in #23188 (IIRC, I cherry-picked the commits from this that PR onto my new branch). See also #23188 (comment).

Sign up for freeto 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.

10 participants

@koubaa@tiran@iritkatriel@vstinner@erlend-aasland@encukou@shihai1991@the-knights-who-say-ni@ezio-melotti@bedevere-bot