Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
error_already_set::what() is now constructed lazily #1895
error_already_set::what() is now constructed lazily #1895
Changes from 19 commits
87d1f6b
e289e0e
f1435c7
60df143
c935b73
b48bc72
115a757
db14dd9
9635e88
dbb3e6b
a692362
ff3185e
95eeaef
00e4852
648e670
244aa52
b09f14d
1d1ec9e
6d0354b
2c5ad0b
68b349a
91bf33f
63dfbb4
d231f15
df6cb30
bdcd95a
225dbae
f8d3ed2
b90bd78
3286964
978bf2a
6c97e68
6523bc2
6c6971f
98aff18
e71d9b9
e9a2e6d
a83028c
e0d8d0e
d32bc91
226cdde
f4a440a
8f6ab3f
4da9968
f181dac
6e781a0
1f8f0ab
2031225
0aee425
1c8398f
a8f7a97
0691d5f
28de959
c7a7146
de84a27
498195a
80b05ba
acdf332
58ad49b
90b2453
dded024
4193375
b020e04
02df6c0
d28c155
7578de9
bab6f66
2ad2285
e3ebb0d
8dff51d
923725a
53f40a0
b77e703
493d751
6d0ec49
6d9188f
8f3d3a6
724ee3d
467ab00
eaa1a48
dbed20a
f021543
125e2d0
b037958
2baa3bc
4c479f4
b4326f9
aaec34e
99af318
e5f028f
8c76360
c7149d8
47f5445
7c08e7f
f746a3d
25e4cc9
70b870a
d425bb8
65aaa4c
19bb595
d07b5ba
f6bf5aa
cc5d76a
2b31e4d
73cffe4
2c5dd19
14be38c
db97ce7
42d2e1a
6417a76
a62b3d7
c54309c
c786796
61bb513
bb3f24c
087ef29
bee5fb4
bea0c9f
d6d371a
eec0547
ec1a2c0
c39ebd5
286023b
156b57b
6f175ac
71f33a8
0739d4c
80dfaea
a7ba693
9854589
d2e78b0
0e50c45
4e06fb1
f892cce
53e29f4
8e145c0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there a discussion about this already?
It would seem better to also do this lazily.
https://docs.python.org/3/c-api/exceptions.html#c.PyErr_NormalizeException
Quote: The delayed normalization is implemented to improve performance.
The documentation isn't clear about it, but I know from recent experience that this function can fail. I believe we need to check
PyErr_Occurred()
withpybind11_fail
, or evenstd::terminate
(similar to how an exception during unwinding causes process termination). Again from recent experience, original errors getting masked by errors in the error handling can be extremely time consuming to debug. Better kill the process, reporting as much information as possible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rwgk See #1895 (comment) . Happy to move it, it was to help a pretty rare edge case for consistency anyhow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm ... OK, let's keep this for later.
Based on the very-real-to-me "pain" experience I mentioned earlier, I'm actually pretty worried about the case that
PyErr_NormalizeException()
fails, but apparently that's not handled here, or in type_caster_base.h. But then again, that's an existing issue that we (me) can look into separately.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
std::runtime copy ctor is noexcept so I am unsure if we should specify that here. The only real issue is the allocation of the new std::string could throw I suppose.