Support for string_view slots#17
Conversation
matusnovak
left a comment
matusnovak
left a comment
There was a problem hiding this comment.
Yeah, it is a lot of templating magic, but this looks pretty good! If you update the documentation I can merge it.
|
|
||
| template <> inline std::string_view getSlot(WrenVM* vm, int idx) { | ||
| validate<WrenType::WREN_TYPE_STRING>(vm, idx); | ||
| return std::string_view(wrenGetSlotString(vm, idx)); |
There was a problem hiding this comment.
This may be very dangerous. The string_view would not own the memory.
Therefore, if you have a C++ function that accepts string_view, and stores the string_view for a future use, then that may cause a segmentation fault. As far as I know Wren only guarantees that the raw bytes of that slot (string) is valid only during the call of the C++ function. The garbage collection may run after calling the function and the string_view points to an unknown memory.
This is fine the other way around. Wren makes a copy of the string/string_view you give it.
I am fine with accepting the risk, but could you update the documentation here? -> https://github.com/matusnovak/wrenbind17/blob/master/docs/content/Tutorial/types.md Maybe adding some section explaining this danger. I don't mind the exact paragraphs, I can leave that to you. Plus a brief comment on this line that yes it is a risk.
There was a problem hiding this comment.
Sorry for the late response, but i got distracted with other projects and completly forgot about this PR, until i checked out my old project.
I can update the paragraph, but i think std::string_view does never own memory does it?
So the risk should probably be known to the user.
2 participants
This adresses #16
These changes do work for me, however i have no idea if it includes all places required (there's too much template magic going on for me :P )