Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-36793: Remove unneeded __str__ definitions. #13081

Merged
merged 2 commits into from
May 6, 2019

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented May 4, 2019

Classes that define __str__ the same as __repr__ can
just inherit it from object.

https://bugs.python.org/issue36793

Classes that define __str__ the same as __repr__ can
just inherit it from object.
@skrah
Copy link
Contributor

skrah commented May 4, 2019

Hm, what is the benefit of this? I find the existing code much clearer.

@serhiy-storchaka
Copy link
Member Author

The benefit is consistency. When you define __repr__ in a subclass of list or dict or in any user class, this will affect the result of str(). But int, float and complex are exceptional. There were reasons to be exceptional in Python 2, but this is no longer needed in Python 3.

Similarly, we usually do not implement __ne__ if define __eq__, because the default implementation of __ne__ falls back to __eq__.

@skrah
Copy link
Contributor

skrah commented May 4, 2019

But falling back is a bit slower, right (I did not look at the relevant code)?

@serhiy-storchaka
Copy link
Member Author

The overhead is very small. It is just few memory reads, comparison an one C function call. It is comparable with Py_INCREF/Py_DECREF.

static PyObject *
object_str(PyObject *self)
{
    unaryfunc f;

    f = Py_TYPE(self)->tp_repr;
    if (f == NULL)
        f = object_repr;
    return f(self);
}

In the worst case, for booleans, where __repr__() just returns a precreated string, I got the difference around 1.5 times of the standard deviation:

$ ./python -m perf timeit -s "a = [True] * 10**3" -- "list(map(str, a))"
Unpatched:  Mean +- std dev: 94.3 us +- 2.8 us
Patched:    Mean +- std dev: 98.1 us +- 3.2 us

In other cases it is smaller than the standard deviation (actually there is a large probability that you get better result with the patched version).

$ ./python -m perf timeit -s "a = [12345] * 10**3" -- "list(map(str, a))"
Unpatched:  Mean +- std dev: 183 us +- 4 us
Patched:    Mean +- std dev: 185 us +- 4 us
$ ./python -m perf timeit -s "a = [12.345] * 10**3" -- "list(map(str, a))"
Unpatched:  Mean +- std dev: 451 us +- 12 us
Patched:    Mean +- std dev: 447 us +- 13 us
$ ./python -m perf timeit -s "a = [12.345j] * 10**3" -- "list(map(str, a))"
Unpatched:  Mean +- std dev: 670 us +- 17 us
Patched:    Mean +- std dev: 669 us +- 19 us

It is insignificant with the time of creating new string.

@skrah
Copy link
Contributor

skrah commented May 4, 2019

@serhiy-storchaka Thanks, I have no objections then. Probably people do expect __repr__ to behave like you said.

Personally, I still always define both __eq__and __ne__. :)

@@ -1419,8 +1419,7 @@ def __repr__(self):
e = ''
return '%s(%i bytes read%s)' % (self.__class__.__name__,
len(self.partial), e)
def __str__(self):
return repr(self)
__str__ = object.__str__
Copy link
Contributor

Choose a reason for hiding this comment

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

(just because I'm curious) in some cases this line was removed, in some cases it was moved to object.__str__ -- is there a heuristic you used here or ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is removed if no parent class except object defines __str__. But if it was changed in the parent class (in BaseException in this case) we need to set it explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks for the clarification 👍

Co-Authored-By: serhiy-storchaka <storchaka@gmail.com>
@gpshead gpshead changed the title bpo-36793: Remove unneded __str__ definitions. bpo-36793: Remove unneeded __str__ definitions. May 5, 2019
@serhiy-storchaka serhiy-storchaka merged commit 96aeaec into python:master May 6, 2019
@serhiy-storchaka serhiy-storchaka deleted the no-redundant-str branch May 6, 2019 19:29
CuriousLearner added a commit to CuriousLearner/cpython that referenced this pull request May 7, 2019
* master: (1204 commits)
  bpo-31855: unittest.mock.mock_open() results now respects the argument of read([size]) (pythonGH-11521)
  Forbid creating of stream objects outside of asyncio (python#13101)
  bpo-35925: Skip SSL tests that fail due to weak external certs. (pythonGH-13124)
  Fix rst formatting for several links in ssl documentation (pythonGH-13133)
  bpo-36542: Allow to overwrite the signature for Python functions. (pythonGH-12705)
  bpo-36793: Remove unneeded __str__ definitions. (pythonGH-13081)
  bpo-36766: Typos in docs and code comments (pythonGH-13116)
  bpo-36275: enhance documentation for venv.create() (pythonGH-13114)
  Clarify the download unit in the download section (pythonGH-13122)
  bpo-30668: add missing word in license.rst (pythonGH-13115)
  Unroll import-team in CODEOWNERS (python#13118)
  bpo-36594: Fix incorrect use of %p in format strings (pythonGH-12769)
  bpo-36798: Updating f-string docs for := use case (pythonGH-13107)
  Update wsgiref.rst (python#10488)
  Doc/c-api/exceptions.rst: fix grammar (python#12091)
  bpo-36811: Fix a C compiler warning in _elementtree.c. (pythonGH-13109)
  Only count number of members once (python#12691)
  bpo-16024: Doc cleanup regarding path_fd, dir_fd, follow_symlinks (pythonGH-5505)
  bpo-36791: Safer detection of integer overflow in sum(). (pythonGH-13080)
  bpo-33530: Implement Happy Eyeballs in asyncio, v2 (pythonGH-7237)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants