Skip to content

bpo-32630: Use contextvars in the decimal module#5278

Merged
1st1 merged 4 commits into
python:masterfrom
1st1:dec
Jan 27, 2018
Merged

bpo-32630: Use contextvars in the decimal module#5278
1st1 merged 4 commits into
python:masterfrom
1st1:dec

Conversation

@1st1

@1st1 1st1 commented Jan 23, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@1st1
1st1 requested review from skrah and vstinner January 23, 2018 06:58
@1st1
1st1 requested a review from rhettinger as a code owner January 23, 2018 06:58
njsmith
njsmith reviewed Jan 23, 2018
View reviewed changes

@njsmith njsmith left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe I missed it, but I don't see anything to make with localcontext(...) use the token to reset?

Comment thread Modules/_decimal/_decimal.c Outdated

@njsmith njsmith Jan 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The comment right about this line is wrong now.

Comment thread Modules/_decimal/_decimal.c Outdated

@njsmith njsmith Jan 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't use the thread state dictionary at all anymore?

Comment thread Modules/_decimal/_decimal.c Outdated

@njsmith njsmith Jan 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"thread local" is a bit confusing here.

@1st1

1st1 commented Jan 23, 2018

Copy link
Copy Markdown
Member Author

Maybe I missed it, but I don't see anything to make with localcontext(...) use the token to reset?

Correct, I didn't use the var.reset() mechanism in this patch to make it as minimal as possible and also make it easier for Stefan to review.

For 3.7 it doesn't really matter if var.reset() is used or not since we don't have chained lookups yet. When we have them, we can just update decimal to use var.reset() to be fully compatible.

@1st1
1st1 force-pushed the dec branch 3 times, most recently from 782042f to 7f770af Compare January 26, 2018 22:47
@1st1
1st1 merged commit f13f12d into python:master Jan 27, 2018
@1st1
1st1 deleted the dec branch January 27, 2018 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@njsmith njsmith njsmith left review comments
@vstinner vstinner Awaiting requested review from vstinner
@skrah skrah Awaiting requested review from skrah
@rhettinger rhettinger Awaiting requested review from rhettinger

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@1st1 @njsmith @the-knights-who-say-ni @bedevere-bot