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-47000: Make `io.text_encoding()` respects UTF-8 mode by methane · Pull Request #32003 · python/cpython

Conversation

@methane

@methane methane commented Mar 20, 2022 •
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/issue47000

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
methane added 2 commits March 20, 2022 13:00
@methane
bpo-47000: Make io.text_encoding() respects UTF-8 mode.
96eeaeb
@methane
Fix refcounting
b8f751f
@bedevere-bot bedevere-bot added the awaiting core review label Mar 20, 2022
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Mar 20, 2022
methane
methane commented Mar 20, 2022
View reviewed changes
Comment thread Modules/_io/_iomodule.c
return NULL;
}
}
return &_Py_ID(locale);

@methane methane Mar 20, 2022

Copy link
Copy Markdown
Member Author

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.

@ericsnowcurrently We need to incref this until we implement immortal object. Am I correct?

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Mar 22, 2022

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.

Sort of. Currently all statically allocated global objects have their refcount initialized to a really large value. So we haven't been bothering with the incref if we know the object is one of those, which it is in this case. Incref'ing it won't hurt. However, it is effectively unnecessary.

Sorry, something went wrong.

Uh oh!

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

@methane methane Mar 23, 2022

Copy link
Copy Markdown
Member Author

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 am afraid breaking refleak buildbot.
Would you tell my why this return &_Py_ID(locale); don't break refleak test?

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Mar 23, 2022

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.

The refcount of static objects is not directly included in _Py_RefTotal, which is used for refleak detection. (_Py_RefTotal will still ensure that refcount operations are balanced though, regardless of the objects involved.)

Sorry, something went wrong.

Uh oh!

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

@methane methane Mar 23, 2022

Copy link
Copy Markdown
Member Author

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.

(_Py_RefTotal will still ensure that refcount operations are balanced though, regardless of the objects involved.)

So skipping INCREF here might cause in-balance of _Py_RefTotal. Doesn't it break refleak test?

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Mar 23, 2022

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.

Good point. It didn't cause a problem before. I'd have to look into why not.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@methane
make clinic
bafa263
ericsnowcurrently
ericsnowcurrently requested changes Mar 22, 2022
View reviewed changes
Comment thread Modules/_io/_iomodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels Mar 22, 2022
@bedevere-bot

bedevere-bot commented Mar 22, 2022

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@methane @ericsnowcurrently
Update Modules/_io/_iomodule.c
de5867f 
Co-authored-by: Eric Snow <[email protected]>
@ericsnowcurrently ericsnowcurrently mentioned this pull request Mar 23, 2022
bpo-46541: Add a comment about when to use _Py_DECLARE_STR(). #32063
Merged
ericsnowcurrently added a commit that referenced this pull request Mar 23, 2022
@ericsnowcurrently
bpo-46541: Add a Comment About When to Use _Py_DECLARE_STR(). (gh-32063)
21412d0 
In a gh-32003 comment, I realized it wasn't very clear how _Py_DECLARE_STR() should be used.  This changes adds a comment to clarify.

https://bugs.python.org/issue46541
@methane @ericsnowcurrently
Update Modules/_io/_iomodule.c
52045fc 
Co-authored-by: Eric Snow <[email protected]>
@methane
methane requested a review from ericsnowcurrently March 31, 2022 04:45
methane added 3 commits March 31, 2022 14:29
@methane
test: ignore utf-8 vs UTF-8
37f640a
@methane
Update _iomodule.c
addff87
@methane
Update the doc
11c614c
@methane methane changed the title bpo-47000: Make io.text_encoding() respects UTF-8 mode. bpo-47000: Make io.text_encoding() respects UTF-8 mode Apr 3, 2022
methane added 3 commits April 3, 2022 13:33
@methane
Remove trailing spaces.
e6f46fa
@methane
Update docstring
ff8eb27
@methane
make clinic
ce48c4a
@methane
methane merged commit 4216dce into python:main Apr 4, 2022
@methane
methane deleted the textencoding-utf8 branch April 4, 2022 02:46
@bedevere-bot bedevere-bot removed the awaiting changes label Apr 4, 2022
@methane methane mentioned this pull request Apr 10, 2022
Make encoding="locale" uses locale encoding even in UTF-8 mode is enabled. #91156
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

4 participants

@methane @bedevere-bot @ericsnowcurrently @the-knights-who-say-ni
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.