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

docs: Fix errors in string "Explicit conversions" #4658

Merged
merged 1 commit into from
May 9, 2023

Conversation

tjstum
Copy link
Contributor

@tjstum tjstum commented May 9, 2023

Description

PyUnicode_DecodeLatin1 requires you to pass in the error parameter. The code as it is in the docs didn't compile.

There is a reference leak in the example code. PyUnicode_DecodeLatin1 returns a new reference. Calling py::str(PyObject*) calls PyObject_Str, which also returns a new reference. That reference is managed by the py::str constructor (which correctly steals the reference, using the stolen_t constructor), but the original reference returned by PyUnicode_DecodeLatin1 is never decref'd: it never makes it into an object, and it's never manually decremented.

This fixes both of those issues. The code compiles, and I viewed the sphinx docs locally.

`PyUnicode_DecodeLatin1` requires you to pass in the `error`
parameter. The code as it is in the docs didn't compile.

There is a reference leak in the example
code. `PyUnicode_DecodeLatin1` returns a new reference. Calling
`py::str(PyObject*)` calls `PyObject_Str`, which also returns a new
reference. That reference is managed by the `py::str`
constructor (which correctly steals the reference, using the
`stolen_t` constructor), but the original reference returned by
`PyUnicode_DecodeLatin1` is never decref'd: it never makes it into an
`object`, and it's never manually decremented.

This fixes both of those issues. The code compiles, and I viewed the
sphinx docs locally.
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! (I'll merge as soon as I see that the pre-commit checks are happy.)

@rwgk rwgk merged commit cca4c51 into pybind:master May 9, 2023
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label May 9, 2023
@rwgk rwgk removed the needs changelog Possibly needs a changelog entry label May 9, 2023
@tjstum tjstum deleted the strings_docs branch May 9, 2023 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants