Skip to content

pybind11::str::raw_str simplification (for Python 2) #2367

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

pybind11::str::raw_str simplification (for Python 2) #2367

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions include/pybind11/pytypes.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,12 +930,12 @@ class str : public object {
private:
/// Return string representation -- always returns a new reference, even if already a str
static PyObject *raw_str(PyObject *op) {
PyObject *str_value = PyObject_Str(op);
if (!str_value) throw error_already_set();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one line went missing but is important! (Unless this can never fail?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Important to add a test for this as well)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nvm. Just noticed this will be done by the code generated by PYBIND11_OBJECT_CVT ;-)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one line went missing but is important! (Unless this can never fail?)

Oops, sorry, that was an accident (manual cherry-picking). I fixed it quick to get that part out of the way. More changes later.

#if PY_MAJOR_VERSION < 3
PyObject *unicode = PyUnicode_FromEncodedObject(str_value, "utf-8", nullptr);
Py_XDECREF(str_value); str_value = unicode;
PyObject *str_value = PyObject_Unicode(op);
#else
PyObject *str_value = PyObject_Str(op);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have a ton of compatibility macros in common.h. I'd define a new one to avoid this macro branching here. Something like PYBIND11_OBJECT_TO_STRING.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We already have a ton of compatibility macros in common.h. I'd define a new one to avoid this macro branching here. Something like PYBIND11_OBJECT_TO_STRING.

Thanks Boris, I'm inclined to take your suggestions, although it makes it a (slightly) sprawling change.
I just looked: the needed compatibility macro doesn't exist already, I'll have to add it.
Checking with @YannickJadoul and @EricCousineau-TRI : what's your recommendation, local change as-is or adding the compatibility macro?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be fine with a compatibility macro living in common.h. The main concern about macros is some sort of deprecation mechanism when it's no longer necessary, but I'm sure there are several ways to resolve that.

#endif
if (!str_value) throw error_already_set();
return str_value;
}
};
Expand Down