Skip to content

Make CreateInterpreter take std::vector<std::string> #207

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

Open
vgvassilev opened this issue Mar 18, 2024 · 7 comments · May be fixed by #545
Open

Make CreateInterpreter take std::vector<std::string> #207

vgvassilev opened this issue Mar 18, 2024 · 7 comments · May be fixed by #545

Comments

@vgvassilev
Copy link
Contributor

This would allow us to reduce the code bloat in converting strings into const char*. That will lift the responsibility for thinking about lifetime of the const char* from the users on the CppInterOp. See test.cpp in conda-forge/cppinterop-feedstock

Copy link
Contributor

github-actions bot commented Dec 5, 2024

This issue is stale because it has been open for 90 days with no activity.

@github-actions github-actions bot added the stale label Dec 5, 2024
Copy link
Contributor

This issue was closed because it has been inactive for 14 days since being marked as stale.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Dec 20, 2024
@vgvassilev vgvassilev reopened this Dec 20, 2024
@mcbarton mcbarton added to-fix and removed stale labels Dec 20, 2024
@solo-source
Copy link

solo-source commented Mar 31, 2025

@vgvassilev hi, i have come up with a solution for this.
The approach is:

  • Change the function parameters to std::vectorstd::string in CppInterOp.h and CppInterOp.cpp.
  • Inside CreateInterpreter defination, build the argument list as std::string, then convert it to const char* right befor interpreter construction, as it expects const char*.

Few Questions:

@vgvassilev
Copy link
Contributor Author

@vgvassilev hi, i have come up with a solution for this. The approach is:

  • Change the function parameters to std::vectorstd::string in CppInterOp.h and CppInterOp.cpp.
  • Inside CreateInterpreter defination, build the argument list as std::string, then convert it to const char* right befor interpreter construction, as it expects const char*.

Few Questions:

We should probably create an overload for this and deprecate the other interface.

@solo-source
Copy link

Ok, i will do that. Should i also forward the control from the older function to overloaded function

@solo-source
Copy link

solo-source commented Apr 1, 2025

@vgvassilev I have created the overloaded, but i am facing an issue.
//New Overload for CreateInterpreter, takes std::vector<std::string>
CPPINTEROP_API TInterp_t CreateInterpreter( const std::vector<std::string>& Args = {}, const std::vector<std::string>& GpuArgs = {});

[[deprecated("Use the overload that takes std::vector<std::string> instead.")]]
CPPINTEROP_API TInterp_t CreateInterpreter(const std::vector<const char*>& Args, const std::vector<const char*>& GpuArgs);

  • First error while running the unittests was that "error: call of overloaded ‘CreateInterpreter()’ is ambiguous". I was able to fix it by removing the default arguments from deprecated declaration.
  • Second error while running the unittests is "error: call of overloaded ‘CreateInterpreter(, )’ is ambiguous" for call like this "Cpp::CreateInterpreter({}, {"--cuda"});".

I am not sure how to resolve this, i am searching for a solution for this.

@solo-source
Copy link

solo-source commented Apr 1, 2025

@vgvassilev i have opened a PR for this, you can take a look at the changes.

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

Successfully merging a pull request may close this issue.

4 participants