Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-103583: Add codecs and maps to _codecs_* module state#103540

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedApr 14, 2023•
edited
Loading

The current naming is too confusing, since we've also got MultibyteCodec_State
@erlend-aasland
Copy link
ContributorAuthor

I've done a couple of experiments with isolating the codec submodules, but it gets hairy pretty quickly. If any of you see an elegant solution, please shout out.

@erlend-aasland
Copy link
ContributorAuthor

Also: this isvery much WIP. With this PR, I've tried to keep the diff small, but I really do not like all these macro templates. It all feels very fragile to me.

@erlend-aasland
Copy link
ContributorAuthor

Most of the remaining globals are just static data, as far as I can see. Perhaps the solution is as easy as making sure they all live in the const data segment and forget about module state.

@erlend-aasland
Copy link
ContributorAuthor

@corona10, as the code owner for these modules: what do you think about the approach taken in this PR? IMO, I'm not too fond of using macros for so much templating, but I went for it nonetheless since that's the approach already taken by this part of the code base.

@corona10
Copy link
Member

@erlend-aasland Will take a look by this weekend :) Thanks

erlend-aasland reacted with heart emoji

static const MultibyteCodec *codec_list = \
(const MultibyteCodec *)_codec_list;

#define CODEC_STATEFUL(enc) \
Copy link
Member

@corona10corona10Apr 16, 2023•
edited
Loading

Choose a reason for hiding this comment

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

Wow, readability will be frustrating (it's not your fault), I will think about a better way.. :(

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

I know, readability is already frustrating with these modules. If we are to keep the diff down, we just have to go with the macro template style already used. An alternative is to refactor the modules first, but that bears a risk.

@corona10corona10 added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelApr 16, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@corona10 for commit9973a7d 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelApr 16, 2023
@corona10corona10 self-assigned thisApr 16, 2023
@erlend-aaslanderlend-aasland changed the titlegh-103092: Isolate _multibytecodecgh-103583: Isolate _multibytecodecApr 16, 2023
@erlend-aaslanderlend-aasland marked this pull request as ready for reviewApril 16, 2023 21:51
@erlend-aasland
Copy link
ContributorAuthor

Are you happy with these changes as they stand,@corona10?

Copy link
Member

@corona10corona10 left a comment

Choose a reason for hiding this comment

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

lgtm :)

erlend-aasland reacted with heart emoji
@erlend-aasland
Copy link
ContributorAuthor

Thanks for the review, Dong-hee! :)

@erlend-aaslanderlend-aasland changed the titlegh-103583: Isolate _multibytecodecgh-103583: Add codecs and maps to _codecs_* module stateApr 16, 2023
@erlend-aaslanderlend-aasland added the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelApr 16, 2023
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@erlend-aasland for commit65af110 🤖

If you want to schedule another build, you need to add the🔨 test-with-refleak-buildbots label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-refleak-buildbotsTest PR w/ refleak buildbots; report in status section labelApr 16, 2023
@miss-islington
Copy link
Contributor

Status check is done, and it's a success ✅.

@erlend-aaslanderlend-aasland merged commit217911e intopython:mainApr 17, 2023
carljm added a commit to carljm/cpython that referenced this pull requestApr 17, 2023
* main:  Remove `expert-*` from `project-updater` GH workflow (python#103579)pythongh-103583: Add codecs and maps to _codecs_* module state (python#103540)pythongh-48330: address review comments to PR-12271 (python#103209)pythongh-103527: Add multibytecodec.h as make dep for _codecs_* (python#103567)pythongh-103553: Improve `test_inspect`: add more assertions, remove unused (python#103554)pythonGH-103517: Improve tests for `pathlib.Path.walk()` (pythonGH-103518)pythongh-102114: Make dis print more concise tracebacks for syntax errors in str inputs (python#102115)pythonGH-78079: Fix UNC device path root normalization in pathlib (pythonGH-102003)pythongh-101517: Add regression test for a lineno bug in try/except* impacting pdb (python#103547)pythongh-103527: Add make deps for _codecs_* and _multibytecodec (python#103528)pythongh-103532: Fix reST syntax in NEWS entry (pythonGH-103544)pythongh-103532: Add NEWS entry (python#103542)
carljm added a commit to carljm/cpython that referenced this pull requestApr 17, 2023
* superopt:  update generated cases with new comment  review comments  Remove `expert-*` from `project-updater` GH workflow (python#103579)pythongh-103583: Add codecs and maps to _codecs_* module state (python#103540)pythongh-48330: address review comments to PR-12271 (python#103209)pythongh-103527: Add multibytecodec.h as make dep for _codecs_* (python#103567)pythongh-103553: Improve `test_inspect`: add more assertions, remove unused (python#103554)pythonGH-103517: Improve tests for `pathlib.Path.walk()` (pythonGH-103518)pythongh-102114: Make dis print more concise tracebacks for syntax errors in str inputs (python#102115)pythonGH-78079: Fix UNC device path root normalization in pathlib (pythonGH-102003)pythongh-101517: Add regression test for a lineno bug in try/except* impacting pdb (python#103547)pythongh-103527: Add make deps for _codecs_* and _multibytecodec (python#103528)pythongh-103532: Fix reST syntax in NEWS entry (pythonGH-103544)pythongh-103532: Add NEWS entry (python#103542)
@erlend-aaslanderlend-aasland deleted the isolate-multibytecodec branchJuly 4, 2023 22:48
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@corona10corona10corona10 approved these changes

Assignees

@corona10corona10

Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@erlend-aasland@corona10@bedevere-bot@miss-islington
[8]ページ先頭
©2009-2025 Movatter.jp