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.

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

docs: fix error condition in custom type caster example #5416

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

docs: fix error condition in custom type caster example #5416

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
2 changes: 1 addition & 1 deletion docs/advanced/cast/custom.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ type is explicitly allowed.
value.long_value = PyLong_AsLong(tmp);
Py_DECREF(tmp);
/* Ensure return code was OK (to avoid out-of-range errors etc) */
return !(value.long_value == -1 && !PyErr_Occurred());
return !(value.long_value == -1 && PyErr_Occurred());
Copy link
Collaborator

Choose a reason for hiding this comment

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

The error needs to be cleared, too.

A better version of custom.rst file is buried in here:

https://github.com/pybind/pybind11/pull/3862/files#diff-56bf050c9c6d51442eb6561df1438ea1ce5d05d5f3b339f36a72a779155131b0

The ADL approach under #3862 didn't work out, so the parts referring to that need to be removed.

Looking at it with a fresh eye, I think there is still a bug in the cast() implementation: missing error handling again.

Let me see if I can rescue custom.rst and tests/test_docs_advanced_cast_custom.cpp,py from the old PR. Maybe this weekend.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or please let me know if you want to help with the rescuing.

Copy link
Author

Choose a reason for hiding this comment

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

Well, I'd be happy to give a shot at fully sanitizing the custom type caster example. I can add the other missing error checks to this PR, also including the associated modifications of the test.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Awesome, please tag me here when this PR is ready for re-review. It'll be great if you could adopt the doc changes from 3862 as well (excluding the ADL-related bits).

}

/**
Expand Down
Loading