GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-28598: Support __rmod__ for RHS subclasses of str in % string formatting operations by mjpieters · Pull Request #51 · python/cpython

Conversation

mjpieters commented Feb 12, 2017 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

When you use '%s' % SubClassOfStr(), where SubClassOfStr.__rmod__ exists, the reverse operation is ignored as normally such string formatting operations use the PyUnicode_Format() fast path. This patch tests for subclasses of str first and picks the slow path in that case.

Fixes bpo-28598.

Sorry, something went wrong.

Uh oh!

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

All reactions


          Support __rmod__ for RHS subclasses of str in % string formatting ope…

1ef5f55

…rations.

When you use `'%s' % SubClassOfStr()`, where `SubClassOfStr.__rmod__` exists, the reverse operation is ignored as normally such string formatting operations use the `PyUnicode_Format()` fast path. This patch tests for subclasses of `str` first and picks the slow path in that case.

Fixes issue28598.

the-knights-who-say-ni added the CLA signed label

Feb 12, 2017

mjpieters commented Feb 12, 2017

Copy link

Copy Markdown

Contributor Author

Open question: Should this patch be backported to 3.6 and 3.5? It's a bug fix, not a new feature.

All reactions

Sorry, something went wrong.

Uh oh!

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

berkerpeksag changed the title ~~Support __rmod__ for RHS subclasses of str in % string formatting operations~~ bpo-28598: Support __rmod__ for RHS subclasses of str in % string formatting operations

Feb 12, 2017

berkerpeksag commented Feb 12, 2017

Copy link

Copy Markdown

Member

Should this patch be backported to 3.6 and 3.5?

Yes, it should be backported if it's a bug (I haven't investigated myself yet.)

I'm not sure if issues are linked automatically yet.

We don't use issueNNNN anymore. The correct way to link PRs to bugs.p.o is bpo-NNNN.

All reactions

Sorry, something went wrong.

Uh oh!

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

mjpieters commented Feb 12, 2017

Copy link

Copy Markdown

Contributor Author

We don't use issueNNNN anymore. The correct way to link PRs to bugs.p.o is bpo-NNNN.

Ta, also for the title change! Updated the 'fixes' line too.

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka reviewed

Feb 12, 2017

View reviewed changes

serhiy-storchaka left a comment

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.

In general LGTM except few minor details.

And I'm not sure about the style of commit messages now. Should it include issue number at the first line? Should it contain "Patch by YYY." (now the author is specified in metadata)?

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/ceval.c Outdated

		PyNumber_Remainder(dividend, divisor);
		PyObject *res;
		if (PyUnicode_CheckExact(dividend) && !(
		PyUnicode_Check(divisor) && !PyUnicode_CheckExact(divisor))) {

serhiy-storchaka Feb 12, 2017

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.

Too many negations.

I would expand the expression as PyUnicode_CheckExact(dividend) && (!PyUnicode_Check(divisor) || PyUnicode_CheckExact(divisor)).

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Misc/NEWS Outdated

		Core and Builtins
		-----------------

		- Issue #28598: Support __rmod__ for subclasses of str being called before str.__mod__.

serhiy-storchaka Feb 12, 2017

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.

Add "Patch by Martijn Pieters."

Sorry, something went wrong.

Uh oh!

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

All reactions

berkerpeksag added backport to 3.5 labels

Feb 12, 2017


          Address review comments; reduce negations and credit patch

7b19b02

codecov Bot commented Feb 12, 2017

Copy link

Copy Markdown

Codecov Report

Merging #51 into master will decrease coverage by -0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
- Coverage   82.37%   82.37%   -0.01%     
==========================================
  Files        1427     1427              
  Lines      350948   350953       +5     
==========================================
- Hits       289099   289097       -2     
- Misses      61849    61856       +7

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af88e7e...7b19b02. Read the comment docs.

All reactions

Sorry, something went wrong.

Uh oh!

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

serhiy-storchaka approved these changes

Feb 13, 2017

View reviewed changes

brettcannon commented Feb 13, 2017

Copy link

Copy Markdown

Member

Should this really be backported? It seems like a change in semantics more than a bugfix.

All reactions

Sorry, something went wrong.

Uh oh!

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

mjpieters commented Feb 14, 2017

Copy link

Copy Markdown

Contributor Author

@brettcannon This is definitely a bug, not a change in semantics. From the object.__rmod__ documentation:

Note: If the right operand’s type is a subclass of the left operand’s type and that subclass provides the reflected method for the operation, this method will be called before the left operand’s non-reflected method. This behavior allows subclasses to override their ancestors’ operations.

str % subclass_of_str doesn't follow this behavior, and str is the only type that doesn't (everything else goes through the PyNumber_Remainder() slow path). This patch fixes that bug.

All reactions

Sorry, something went wrong.

Uh oh!

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

This was referenced Feb 14, 2017

Bpo28598 backport 3.6 #92

Closed

[backport to 3.5] bpo-28598: Support __rmod__ for RHS subclasses of s… #93

Closed

[3.5] bpo-28598: Support __rmod__ for RHS subclasses of str in % string formatting operations #94

Merged

[backport to 3.6] bpo-28598: Support __rmod__ for RHS subclasses of str in % string formatting operations #95

Merged

serhiy-storchaka added the type-bug An unexpected behavior, bug, or error label

Feb 18, 2017

methane mentioned this pull request

Feb 21, 2017

.gitattribute -> .gitattributes #213

Merged


          Merge branch 'master' into issue28598_str_rmod_fix

f596f01

serhiy-storchaka merged commit d7e6433 into python:master

Feb 23, 2017

zhangyangyu added the needs backport to 2.7 label

Mar 1, 2017

zhangyangyu mentioned this pull request

Mar 1, 2017

[2.7] bpo-28598: Support __rmod__ for RHS subclasses of str in % string formatting operations #366

Merged

Mariatta removed needs backport to 2.7 labels

Mar 1, 2017

mjpieters mentioned this pull request

Mar 1, 2017

[backport to 2.7] bpo-28598: Support __rmod__ for RHS subclasses of str in % string formatting operations #382

Closed

mjpieters deleted the issue28598_str_rmod_fix branch

March 1, 2017 16:55

colesbury referenced this pull request in colesbury/nogil

Oct 6, 2021


          Add a lock around dtoa mult5 code

ec46739

See #51

tendermonster mentioned this pull request

Oct 19, 2022

Multiprocessing returns Array, RawArray of wrong length python 3.10.4 #98447

Closed

jaraco pushed a commit that referenced this pull request

Dec 2, 2022


          Merge the PR with one core dev approval and CI passed. (GH-51)

69f93c6

Allow miss-islington to merge the PR if all of these conditions met:
- All CI passed
- At least one core dev approved
- PR is made by miss-islington

jaraco pushed a commit to jaraco/cpython that referenced this pull request

Feb 17, 2023


          Merge branch 'issue51' into 'master'

bd5afac

Back port latest changes to importlib.resources

Closes python#51

See merge request python-devs/importlib_resources!55

mgmacias95 pushed a commit to mgmacias95/cpython that referenced this pull request

Mar 24, 2023


          Merge pull request python#51 from isidentical/more-simplifications

b41a3b8

oraluben pushed a commit to oraluben/cpython that referenced this pull request

Jul 11, 2023


          feat: bigger CACHE entries for everything, CACHE for JUMP_BACKWARD (p…

…ython#51)

* feat: store bb_test flag in frame to reload during resume

* feat: bigger CACHE entries for everything, CACHE for JUMP_BACKWARD

* nit: increase overallocate factor even more

* feat: remove bb_flag/bb_test and use only the frame's

gvanrossum mentioned this pull request

Aug 22, 2023

heap-use-after-free in _PyFunction_LookupByVersion #108253

Closed

LinanV mentioned this pull request

Oct 23, 2023

Objects/typeobject.c: No such file or directory. #111203

Closed

kcatss mentioned this pull request

Feb 20, 2024

Use After Free at _heapreplace_max #115706

Closed

ngoldbaum mentioned this pull request

Sep 23, 2024

Crash running PyO3 tests with --test-threads=1000 #124375

Closed

This was referenced Feb 11, 2025

segmentfault when pip installing setuptools #129992

Closed

segmentfault when pip installing setuptools #129993

Closed

sergey-miryanov mentioned this pull request

Dec 18, 2025

Observed memory leak in ssl library: Python 3.14 GC issue #142516

Closed

DinoV added a commit to DinoV/cpython that referenced this pull request

Feb 11, 2026


          Merge pull request python#51 from DinoV/fix_merge

80b3925

Fix bad merge

ngoldbaum mentioned this pull request

Mar 6, 2026

Remove deprecated and undocumented function ctypes.SetPointerType #133866

Closed

Reviewers

serhiy-storchaka

serhiy-storchaka approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

type-bug

An unexpected behavior, bug, or error

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.

7 participants

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.

GeezerVison

Conversation

mjpieters commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjpieters commented Feb 12, 2017

Uh oh!

berkerpeksag commented Feb 12, 2017

Uh oh!

mjpieters commented Feb 12, 2017

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Feb 12, 2017

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Feb 12, 2017

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Feb 12, 2017

Codecov Report

Uh oh!

brettcannon commented Feb 13, 2017

Uh oh!

mjpieters commented Feb 14, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mjpieters commented Feb 12, 2017 •

edited

Loading

Uh oh!

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