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

[embind] Fix misindexed template parameters which prevented policies from being applied to class functions or constructors #24053

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

canislupaster
Copy link

Hi, trying to allow_raw_pointers() in a class method bound with embind when a custom signature was given as a template argument to .class_function failed. It's easy to see why by comparing the definition of either RegisterClassMethod or RegisterClassConstructor with their usage:

template<typename ReturnType, typename ThisType, typename... Args>
struct RegisterClassMethod<ReturnType (ThisType, Args...)> {

    template <typename ClassType, typename Callable, typename... Policies>
    static void invoke(const char* methodName,
...

and its invocation in class_function:

using invoker = internal::RegisterClassConstructor<
            typename std::conditional<std::is_same<Signature, internal::DeduceArgumentsTag>::value,
                                      Callable,
                                      Signature>::type>;

invoker::template invoke<ClassType, Policies...>(callable);

which will skip the Callable template argument in some specializations (and Callable will become the return value policy). To fix this, I used the same approach used by the getters/setter PropertyTags (c.f. diff). Please let me know if anything is off.

@sbc100 sbc100 added the embind label Apr 7, 2025
@sbc100 sbc100 requested a review from brendandahl April 7, 2025 18:22
Copy link
Collaborator

@brendandahl brendandahl left a comment

Choose a reason for hiding this comment

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

Looks good. I think you'll need to run
test/runner other.test_minimal_runtime_code_size_hello_embind* --rebaseline
to fix up the code size tests.

@sbc100 sbc100 changed the title (embind) fix misindexed template parameters which prevented policies from being applied to class functions or constructors [embind] Fix misindexed template parameters which prevented policies from being applied to class functions or constructors Apr 8, 2025
@canislupaster
Copy link
Author

I'm not sure if something with my environment is different from CircleCI's (MacOS / Apple Silicon, latest emsdk, no warnings about version mismatches for clang/binaryen/etc), but I get the old/incorrect code size locally. This seems like a terrible solution, but I can substitute the "correct" sizes from #24055 .

@sbc100
Copy link
Collaborator

sbc100 commented Apr 9, 2025

I'm not sure if something with my environment is different from CircleCI's (MacOS / Apple Silicon, latest emsdk, no warnings about version mismatches for clang/binaryen/etc), but I get the old/incorrect code size locally. This seems like a terrible solution, but I can substitute the "correct" sizes from #24055 .

Did you do emsdk instal tot to get the tip-of-tree release?

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

Successfully merging this pull request may close these issues.

3 participants