Skip to content

Conversation

@vsajip
Copy link
Member

@vsajipvsajip commented Sep 22, 2019

Copy link
Member

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

/*
* See bpo-36876: miscellaneous ad hoc statics have been moved here.
*/
struct{

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.

atbol=1;
PyInterpreterState*interp=_PyInterpreterState_GET_UNSAFE();

interp->parser_data.listnode_data.level=0;

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

@vsajipvsajip merged commit 9def81a into python:masterNov 7, 2019
@vsajipvsajip deleted the fix-36876-parser-listnode branch November 7, 2019 10:08
@vstinner
Copy link
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
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
Member

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

@vsajip
Copy link
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
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