-
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
discussion for STL handling #5
base: master
Are you sure you want to change the base?
Conversation
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.
Here's some initial thoughts.
void as_mut_param(std::vector<float>& vec); | ||
void as_const_param(const std::vector<float>& vec); | ||
} | ||
``` |
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.
Enquiring minds wonder if vector<string>
deserves its own discussion
`const_param` just copies into a temporary vector: | ||
```c++ | ||
void foo_as_const_param(float* buffer, int len) { | ||
std::vector tmp{buffer, len}; |
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 assume this a sketch, since a tmp on the stack will bump up against stack size limits, in the case where an implement of vector manages an embedded array, instead of a heap managed block?
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.
Are there any instances where that's the case? That would require a custom allocator, which would change the type of the vector, and AFAICT all vectors in public APIs of the projects we care about use the default allocator?
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.
True. In my mind I was imagining more widespread use, which isn't necessarily the aim. In gamedev allocator replacement is very common; EASTL is architected around the notion that the allocator is externally supplied.
|
||
### Cons | ||
- Still copies in the parameter case | ||
- Works for POD elements only (no vector<string>) |
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.
Another con is that it's really hard to make this pattern have a low time overhead, for many reasons, including cache misses on traversing the heap allocated nodes in the map
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.
Yeah a flat_map or a vector would be better here, but this is just a sketch. Still your point is valid.
### Pros | ||
- It's guaranteed to work and is safe | ||
- Caller is responsible for all allocations | ||
|
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.
do you mean the caller is responsible for inducing all allocations? That's how I read the proposal. I don't read that the caller is responsible for allocations ~ which wouldn't make sense, given that the vector and string can cause allocations themselves independent of what the caller intended.
All of these proposals need to consider allocation boundaries as well. On Windows for example, given Foo.dll and Bar.dll, each bound to unknown standard libraries, we can guarantee that Foo can freely allocate and free a std::string, and that Bar can access the bytes of the string, in a single threaded context. A string allocated by Foo, and deallocated by Bar drops into the realm of undefined behavior, and more typically simply corrupts Foo's heap, and Bar's heap by cross linking freelists.
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.
Yes absolutely, this is one of the main things we're trying to work around here. In the first option yeah, vector absolutely will not work.
```c++ | ||
|
||
// C++ | ||
using VecVariant = std::variant< |
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'm for this option but without the Variant wrapping all the types.
By hand it would be pain, but if you're generating it, it doesn't
matter if you've got VecFloat, VecInt, VecBool etc. Your code gen should be pretty simple also if you're just wrapping std::vector like any other type. If you explicitly instantiate each type, then clang should be able to match the T=float, T=Int etc.
My argument for this is, you'll get a little more safety (can't call get_float on a VecBool), and you'll be able to wrap vectors of non standard types, std::vector<SdfLayer> as VecSdfLayer
.
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.
Yeah that's what I'm leaning towards too. I'll have to generate another source/header pair alongside the binding sources in order to create each type. Generating all these is going to be a project itself but I'll give it a go for the vector case now and see where I get to.
|
||
### Cons | ||
- Adding new types just for the binding layer | ||
- Unsafe if wrapper object is copied in C then passed by value back to C++ |
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.
You can't avoid this. Non trivial copies in C have to translate to explicit copy constructor calls in c++.
But:
- In rust that goes into Clone trait
- In python your object is going to be on the heap anyway.
This RFC discusses how we handle standard library types such as vector and string. I'm currently leaning towards the last option.