◐ Shell
reader mode source ↗
Skip to content

bpo-36876: Moved Parser/listnode.c statics to interpreter state.#16328

Merged
vsajip merged 2 commits into
python:masterfrom
vsajip:fix-36876-parser-listnode
Nov 7, 2019
Merged

bpo-36876: Moved Parser/listnode.c statics to interpreter state.#16328
vsajip merged 2 commits into
python:masterfrom
vsajip:fix-36876-parser-listnode

Conversation

@vsajip

@vsajip vsajip commented Sep 22, 2019

Copy link
Copy Markdown
Member

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

Hide comment

mostly LGTM

I have left just a few small suggestions to consider.

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

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

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

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

@vstinner

Copy link
Copy Markdown
Member

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

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

Labels

skip news type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants