Skip to content

Conversation

brendandahl
Copy link
Collaborator

Using pointers with val worked inconsistently before where:

  Foo f;
  Foo* p = &f;
  val v1(p); // works fine
  val v2(&f); // fails

The pointer working above was probably a mistake[1] and was caused by TypeID normalizing the types differently than how BindingType does. This patch picks up the work done previously[2] to enforce that types are normalized consistently.

In the above example both will now require a pointer policy e.g. (val v(p, allow_raw_pointers()).

[1]#7292 (comment) [2]https://github.com/yeputons/emscripten/tree/fix-7292-embind-type-normalize

@brendandahl brendandahl requested a review from sbc100 April 23, 2025 17:47
})
def test_embind_no_raw_pointers(self, filename):
stderr = self.expect_fail([EMCC, '-lembind', test_file(filename)])
self.assertContained('Implicitly binding raw pointers is illegal.', stderr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm almost tempted to say you should just write 3 separate tests here (since each one is only two lines), but I'm not sure it makes any difference really.

@brendandahl
Copy link
Collaborator Author

Running some tests in g3 before I land this...

@brendandahl
Copy link
Collaborator Author

Looked good on g3. Merging.

Using pointers with val worked inconsistently before where:

```
  Foo f;
  Foo* p = &f;
  val v1(p); // works fine
  val v2(&f); // fails
```

The pointer working above was probably a mistake[1] and was caused by
TypeID normalizing the types differently than how BindingType does. This
patch picks up the work done previously[2] to enforce that types are
normalized consistently.

In the above example both will now require a pointer policy e.g.
`(val v(p, allow_raw_pointers())`.

[1]emscripten-core#7292 (comment)
[2]https://github.com/yeputons/emscripten/tree/fix-7292-embind-type-normalize
@brendandahl brendandahl force-pushed the embind-canon-type branch from 88e234a to ff021f9 Compare May 2, 2025 19:30
@brendandahl brendandahl merged commit c31159f into emscripten-core:main May 2, 2025
2 of 13 checks passed
brendandahl added a commit to brendandahl/emscripten that referenced this pull request Jul 29, 2025
After emscripten-core#24175, `val` required a pointer policy when creating a `val` with
a pointer. In `val.set(key, value)`, a temporary `val` object is created,
but there was no way to set the policy from user code. This patch allows
the user to pass in `allow_raw_pointers()` to enable this.
brendandahl added a commit that referenced this pull request Jul 31, 2025
)

After #24175, `val` required a pointer policy when creating a `val` with
a pointer. In `val.set(key, value)`, a temporary `val` object is
created, but there was no way to set the policy from user code. This
patch allows the user to pass in `allow_raw_pointers()` to enable this.
brendandahl added a commit to brendandahl/emscripten that referenced this pull request Jul 31, 2025
…w_`.

After emscripten-core#24175, pointer policies were required in more places, but there
was no way to pass the policy into val's `call`, `operator()`, and `new_`
methods and use pointers.

All of these methods take variadic template arguments already
which made passing separate variadic policy arguments challenging. I used
some C++14/17 tricks to separate the regular arguments and policy
arguments.
brendandahl added a commit to brendandahl/emscripten that referenced this pull request Aug 27, 2025
…w_`.

After emscripten-core#24175, pointer policies were required in more places, but there
was no way to pass the policy into val's `call`, `operator()`, and `new_`
methods and use pointers.

All of these methods take variadic template arguments already
which made passing separate variadic policy arguments challenging. I used
some C++14/17 tricks to separate the regular arguments and policy
arguments.
brendandahl added a commit to brendandahl/emscripten that referenced this pull request Sep 4, 2025
…w_`.

After emscripten-core#24175, pointer policies were required in more places, but there
was no way to pass the policy into val's `call`, `operator()`, and `new_`
methods and use pointers.

All of these methods take variadic template arguments already
which made passing separate variadic policy arguments challenging. I used
some C++14/17 tricks to separate the regular arguments and policy
arguments.
brendandahl added a commit to brendandahl/emscripten that referenced this pull request Sep 16, 2025
…w_`.

After emscripten-core#24175, pointer policies were required in more places, but there
was no way to pass the policy into val's `call`, `operator()`, and `new_`
methods and use pointers.

All of these methods take variadic template arguments already
which made passing separate variadic policy arguments challenging. I used
some C++14/17 tricks to separate the regular arguments and policy
arguments.
brendandahl added a commit to brendandahl/emscripten that referenced this pull request Sep 29, 2025
…w_`.

After emscripten-core#24175, pointer policies were required in more places, but there
was no way to pass the policy into val's `call`, `operator()`, and `new_`
methods and use pointers.

All of these methods take variadic template arguments already
which made passing separate variadic policy arguments challenging. I used
some C++14/17 tricks to separate the regular arguments and policy
arguments.
brendandahl added a commit that referenced this pull request Sep 30, 2025
…24832)

After #24175, pointer policies were required in more places, but there
was no way to pass the policy into val's `call`, `operator()`, and
`new_`
methods and use pointers.

All of these methods take variadic template arguments already
which made passing separate variadic policy arguments challenging. 
C++17 is required to separate the regular arguments and policy
arguments. For older versions of C++, we don't allow policy arguments
and just allow pointers by default (too match old behavior).

Fixes #24398
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.

2 participants