Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
`error_already_set` is more complicated than it needs to be, partly because it manages reference counts itself rather than using `py::object`, and partly because it tries to do more exception clearing than is needed. This commit greatly simplifies it, and fixes #927. Using `py::object` instead of `PyObject *` means we can rely on implicit copy/move constructors. The current logic did both a `PyErr_Clear` on deletion *and* a `PyErr_Fetch` on creation. I can't see how the `PyErr_Clear` on deletion is ever useful: the `Fetch` on creation itself clears the error, so the only way doing a `PyErr_Clear` on deletion could do anything if is some *other* exception was raised while the `error_already_set` object was alive--but in that case, clearing some other exception seems wrong. (Code that is worried about an exception handler raising another exception would already catch a second `error_already_set` from exception code). The destructor itself called `clear()`, but `clear()` was a little bit convoluted: it called `restore()` to restore the currently captured error, but then immediately cleared it. The only point seems to be because `PyErr_Restore` decrements the reference counts on the held objects, but we can simply do that manually. (Python documentation suggests that `PyErr_Fetch` doesn't have to be matched with a `PyErr_Restore`). This updates the code to simply release the references on the three objects (preserving the gil acquire). `clear()`, however, also had the side effect of clearing the current error, even if the current `error_already_set` didn't have a current error (e.g. because of a previous `restore()` or `clear()` call). I don't really see how clearing the error here can ever actually be useful: the only way the current error could be set is if you called `restore()` (in which case the current stored error-related members have already been released), or if some *other* code raised the error, in which case `clear()` on *this* object is clearing an error for which it shouldn't be responsible. Neither of those seem like intentional or desirable features, and manually requesting deletion of the stored references similarly seems pointless, so I've just made `clear()` an empty method and marked it deprecated. This also fixes a minor potential issue with the destruction: it is technically possible for `value` to be null (though this seems likely to be rare in practice); this updates the check to look at `type` which will always be non-null for a `Fetch`ed exception.
- Loading branch information
Showing
5 changed files
with
39 additions
and
40 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters