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

Use original name for templated global function #153

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

Conversation

varunagrawal
Copy link
Collaborator

Fixes #148

@varunagrawal varunagrawal requested a review from gchenfc August 5, 2022 22:21
@varunagrawal varunagrawal self-assigned this Aug 5, 2022
@varunagrawal varunagrawal requested a review from ProfFan August 8, 2022 14:51
Copy link
Member

@gchenfc gchenfc left a comment

Choose a reason for hiding this comment

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

Correct me if I'm wrong, but isn't this API breaking since every templated global function name will change?

Also, is it possible that this might cause more overload resolution issues with implicitly convertible types or types that generally are difficult for pybind/matlab to distinguish? e.g.

template <typename T = {double, int>
void function(const T& arg);

or

template <typename T = {std::vector<double>, gtsam::KeyVector>
void function(const T& arg);

?

@varunagrawal
Copy link
Collaborator Author

True it is API breaking, but it follows with the design @dellaert wants from #148.

As for the second point, that would be the case for the current version of the wrapper as well. double and int is not a good example since they correspond to float and int in python, but functionInt and functionSize_t would still have the same behavior. Plus we already have overloading like this working for the Values class in GTSAM.

@ProfFan
Copy link
Collaborator

ProfFan commented Aug 10, 2022

Overload resolution is tricky and I think what Frank wants (based on our last talk) is we explicitly define these typed overloads in the .i files and leave most overloads automatically resolved. I don't fully agree but it is a reasonable plan.

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.

Free function templating does not work (as expected)
3 participants