-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[smart_holder] Change throw_if_uninitialized_or_disowned_holder()
to also show C++ type name.
#4977
[smart_holder] Change throw_if_uninitialized_or_disowned_holder()
to also show C++ type name.
#4977
Conversation
@karagog could you please help reviewing this PR? |
if (!holder().is_populated) { | ||
pybind11_fail("Missing value for wrapped C++ type:" | ||
" Python instance is uninitialized."); | ||
throw value_error(missing_value_msg + clean_type_id(typeid_name) |
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.
I dont' suppose you can use f-strings?
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.
Interesting idea, but no. We don't have f-strings in C++ (I never thought about that before, not sure how feasible that is), and compared to relying on std::string
operator+
, the overhead of using the Python C API is not something I'd want to have in an error handling code path like here.
@@ -9,23 +9,19 @@ | |||
|
|||
@pytest.mark.xfail("env.PYPY", reason="gc after `del field` is apparently deferred") | |||
@pytest.mark.parametrize("m_attr", ["m_valu_readonly", "m_valu_readwrite"]) | |||
def test_valu_getter(msg, m_attr): | |||
def test_valu_getter(m_attr): |
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.
nit: this function name is apparently misspelled
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.
It's intentional, see comment at top of this file, which points here:
pybind11/tests/test_class_sh_property.cpp
Lines 1 to 4 in feed8b1
// The compact 4-character naming matches that in test_class_sh_basic.cpp | |
// Variable names are intentionally terse, to not distract from the more important C++ type names: | |
// valu(e), ref(erence), ptr or p (pointer), r = rvalue, m = mutable, c = const, | |
// sh = shared_ptr, uq = unique_ptr. |
(The original idea was actually to make this code in test_class_sh_basic.cpp more readable.)
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.
LGTM
Thanks for the review! |
Description
To aid debugging.
Suggested changelog entry: