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.

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

Add exception to assert introduced in #549 when the type caster of U* can produce the actual underlying object U #608

Merged
merged 3 commits into from
Jun 10, 2024

Conversation

francesco-ballarin
Copy link
Contributor

Follow up to discussion #605.

I confirmed with my downstream project that the patch does indeed fix the regression introduced in nanobind 2.0.
Please note that, even though the commit has my name and email address, the actual patch was provided by @oremanj.

… can produce the actual underlying object U
@@ -123,7 +124,9 @@ NB_INLINE uint8_t flags_for_local_caster(uint8_t flags) noexcept {
into storage owned by the caster, which won't live long enough.
Exception: the 'char' caster produces a result that points to
storage owned by the incoming Python 'str' object, so it's OK. */
static_assert(!is_ref || std::is_same_v<T, const char*>,
static_assert(!is_ref || std::is_same_v<T, const char*> ||
std::conjunction_v<std::is_pointer<T>,
Copy link
Owner

Choose a reason for hiding this comment

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

Can you replace this with ordinary constexpr boolean expressions? (&&). Conjunction_v is unnecessarily heavy.

@wjakob
Copy link
Owner

wjakob commented Jun 7, 2024

You need is_pointer_v, etc.

@wjakob
Copy link
Owner

wjakob commented Jun 7, 2024

What would be the best way to explain this change in the changelog @oremanj ?

@francesco-ballarin
Copy link
Contributor Author

You need is_pointer_v, etc.

Hopefully after four attempts I've got it right :(

@oremanj
Copy link
Contributor

oremanj commented Jun 7, 2024

I originally used conjunction because I was worried about potentially forming an invalid expression if T was not a pointer type, but on second thought I don’t think it helps or matters.

Possible changelog entry: “nanobind no longer prevents casting to a C++ container of pointers T* where T is a type with a user-defined type caster if the caster seems to operate by extracting a T* from the Python object rather than a T.” with a link to the issue that prompted this PR for more context.

I’m mostly offline until Monday and will respond to this in more depth at that time if needed.

@francesco-ballarin
Copy link
Contributor Author

Thanks @oremanj , I've added what you suggested to the changelog.

@wjakob wjakob merged commit c5ae2a3 into wjakob:master Jun 10, 2024
24 checks passed
@wjakob
Copy link
Owner

wjakob commented Jun 10, 2024

Merged, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants