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

@vsajipvsajip commented Sep 22, 2019

Copy link
Copy Markdown
Member

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

mostly LGTM

I have left just a few small suggestions to consider.

Comment threadInclude/internal/pycore_pystate.h Outdated
Comment threadInclude/internal/pycore_pystate.h Outdated
/*
* See bpo-36876: miscellaneous ad hoc statics have been moved here.
*/
struct {

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.

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.

Comment threadParser/listnode.c Outdated
atbol = 1;
PyInterpreterState *interp = _PyInterpreterState_GET_UNSAFE();

interp->parser_data.listnode_data.level = 0;

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.

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. :)

@vsajip
vsajip merged commit 9def81a into python:masterNov 7, 2019
@vsajip
vsajip deleted the fix-36876-parser-listnode branch November 7, 2019 10:08
@vstinner

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

Copy link
Copy Markdown
MemberAuthor

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

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

Copy link
Copy Markdown
MemberAuthor

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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip newstype-featureA feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@vsajip@vstinner@ericsnowcurrently@the-knights-who-say-ni@bedevere-bot