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-36876: Moved Parser/listnode.c statics to interpreter state. by vsajip · Pull Request #16328 · python/cpython

Conversation

@vsajip

@vsajip vsajip commented Sep 22, 2019 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

https://bugs.python.org/issue36876

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vsajip
Moved Parser/listnode.c statics to interpreter state.
3c82f2e
@vsajip vsajip added type-feature A feature request or enhancement skip news labels Sep 22, 2019
@vsajip
vsajip requested a review from ericsnowcurrently September 22, 2019 04:41
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 22, 2019
@bedevere-bot bedevere-bot added the awaiting core review label Sep 22, 2019
ericsnowcurrently
ericsnowcurrently approved these changes Nov 1, 2019
View reviewed changes

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

mostly LGTM

I have left just a few small suggestions to consider.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/internal/pycore_pystate.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_pystate.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Include/internal/pycore_pystate.h
/*
* See bpo-36876: miscellaneous ad hoc statics have been moved here.
*/
struct {

@ericsnowcurrently ericsnowcurrently Nov 1, 2019

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.

If you think there will be any other parser/compiler-related state to move to PyInterpreterState then consider making this struct stand-alone and perhaps even creating pycore_compiler.h (or pycore_parser.h) for it. That way the code doesn't get too cluttered.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Parser/listnode.c Outdated
atbol = 1;
PyInterpreterState *interp = _PyInterpreterState_GET_UNSAFE();

interp->parser_data.listnode_data.level = 0;

@ericsnowcurrently ericsnowcurrently Nov 1, 2019

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 was going to point to the advantage of having stand-alone structs for places like this (so there isn't so much going on with each line). However, I actually kind of like how the line captures the indirection clearly. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Nov 1, 2019
@vsajip
Addressed review comments.
4c88f82
@vsajip
vsajip merged commit 9def81a into python:master Nov 7, 2019
@vsajip
vsajip deleted the fix-36876-parser-listnode branch November 7, 2019 10:08
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 7, 2019
@vstinner

vstinner commented Nov 7, 2019

Copy link
Copy Markdown
Member 
interp = _PyInterpreterState_GET_UNSAFE();

You can add listnode or parser variable to avoid having to write "interp->parser.listnode". But for that, you need to name the structure in pycore_pystate.h.
All reactions

Sorry, something went wrong.

Uh oh!

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

vsajip commented Nov 7, 2019

Copy link
Copy Markdown
Member Author

I'm not quite sure what you mean. The change was specifically to move those variables into the interpreter state rather than stand-alone variables, as they were previously.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Nov 8, 2019

Copy link
Copy Markdown
Member

I suggest to add parser = &interp->parser; and then use parser instead of interp. Or something similar with listnode.
All reactions

Sorry, something went wrong.

Uh oh!

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

vsajip commented Nov 8, 2019

Copy link
Copy Markdown
Member Author

Ah, OK, I understand now :-)

Not sure what an additional variable buys in this case. I agree, if readability were adversely affected, it would be worth doing. As it is, it seems readable enough.
All reactions

Sorry, something went wrong.

Uh oh!

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

vstinner commented Nov 12, 2019

Copy link
Copy Markdown
Member

It was just a minor remark for readability. You're free to ignore it ;-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019
@vsajip Jake Taylor
bpo-36876: Moved Parser/listnode.c statics to interpreter state. (pyt…
d60dcb1 
…honGH-16328)
shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@vsajip @shihai1991
bpo-36876: Moved Parser/listnode.c statics to interpreter state. (pyt…
9e1a6fe 
…honGH-16328)
@ericsnowcurrently ericsnowcurrently mentioned this pull request Apr 10, 2022
[subinterpreters] Global C variables are a problem #81057
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

skip news type-feature A feature request or enhancement

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.

5 participants

@vsajip @vstinner @ericsnowcurrently @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.