-
Notifications
You must be signed in to change notification settings - Fork 2
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
[291] Nanobind for QueryAnswer and DASNode #299
base: master
Are you sure you want to change the base?
Conversation
//TODO: nanobind is failing to bind the handles field | ||
// error: invalid conversion from 'const char* const*' to 'char' | ||
// .def_ro("handles", &HandlesAnswer::handles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedro-costa-techlabs const char*
(or any other type of pointer) in nanobind is always challenging and should be avoided.
This particular case is even more complicated because handles
is an array of pointers, so the binding implementation must translate it to a format that nanobind knows how to deal with, as follows:
#include <nanobind/stl/vector.h>
...
.def_ro(
"handles",
[](const HandlesAnswer& self) -> const vector<string> {
vector<string> handles;
for(size_t i = 0; i < self.handles_size; i++) {
handles.emplace_back(self.handles[i]);
}
return handles;
})
nb::class_<HandlesAnswer, QueryAnswer>(m, "HandlesAnswer") | ||
.def(nb::init<>()) | ||
.def(nb::init<double>(), "importance"_a) | ||
.def(nb::init<const char*, double>(), "handle"_a, "importance"_a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could also require a special treatment for const char* handle
:
.def(
"__init__",
[](HandlesAnswer& self, const string& handle, double importance) {
new (&self) HandlesAnswer(handle.c_str(), importance);
},
"handle"_a,
"importance"_a)
|
||
// RemoteIterator.h | ||
nb::class_<RemoteIterator<HandlesAnswer>>(m, "RemoteIterator") | ||
.def(nb::init<string>(), "local_id"_a) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.def(nb::init<string>(), "local_id"_a) | |
.def(nb::init<const string&>(), "local_id"_a) |
Please revisit the other functions/constructors to ensure that the binding is expecting/passing in the right types, including aspect modifiers like const
, &
, etc.
closes #291