Fixed memory leak with String objects#307
Conversation
|
You aren't obeying the Clang format and therefore your build fails. |
|
Sorry, I didn't notice, but I'll fix the issues. |
aqnuep
force-pushed
the
string_memory_leak_fix
branch
from
fc7239a to
a3e7696
Compare
|
Fixed. Thanks for taking a look at this! |
|
Should this pull request be merged now that #333 has been merged? |
|
Sounds like a plan if someone can resolve the conflicts. |
aqnuep
force-pushed
the
string_memory_leak_fix
branch
3 times, most recently
from
b1ce289 to
d995f93
Compare
|
Should be conflict-free now, but let's see the checks. |
|
About testing, I'm thinking of this #413 |
|
I'm trying to reproduce a case of memory leak, but not getting one so far. void NativeTest::test_create_strings() {
String::num_int64(42);
}What code should I use to reproduce the original problem? After some digging in Godot codebase, it turns out Although, it is true that from an API standpoint it should be used properly, even if default construction happen to allocate nothing. |
|
|
||
| void String::operator+=(const String &s) { | ||
| _godot_string = godot::api->godot_string_operator_plus(&_godot_string, &s._godot_string); | ||
| *this = String(godot::api->godot_string_operator_plus(&_godot_string, &s._godot_string)); |
There was a problem hiding this comment.
Creating a new string on += looses a big advantage += normally has: exploiting capacity, instead of reallocating an entire buffer for every line, or every character being inserted at the end. If it's not available in the C API we may have to ask for it.
Can't do much in this PR but need to keep track of it
There was a problem hiding this comment.
Yes, I'm aware of it. Unfortunately we have to live with that limitation for now AFAICT.
| return godot::api->godot_string_naturalnocasecmp_to(&_godot_string, &p_str._godot_string); | ||
| } | ||
|
|
||
| String String::dedent() const { |
There was a problem hiding this comment.
Where did those functions go?
They were added after b895d3c but maybe you didn't rebase on the right version?
There was a problem hiding this comment.
Sorry about that. It looks like somehow that got removed unintentionally during rebasing and I didn't notice it. I'll fix it in a moment.
There was a problem hiding this comment.
FYI, not sure what caused the merging issue here, because in my local version those new functions were untouched. Anyway, I did a forced push to resolve this problem.
The problematic API that caused memory leaks was the "+=" operator which, unlike all other modification operations, replaced the "_godot_string" member's value without first destroying its previously held godot_string object. All the other APIs I modified return a new String object, so there my changes only really make a refactoring effort for simpler and more unified code (previously things were done differently in a few cases). However, the "+=" operator does in-place modification of the String object (needing an in-place replacement of the underlying godot_string object due to lack of a "+=" operator in the C API), and the previous code didn't take care of destroying the old value held, thus leaking that memory at every call. |
The member _godot_string should never be straight out overwritten ever without first destroying the underlying string object's memory. This change solves the problem through the introduction of a new private constructor to create String objects with a pre-existing godot_string handle.
aqnuep
force-pushed
the
string_memory_leak_fix
branch
from
d995f93 to
0939d0f
Compare
|
Thanks! |
5 participants
The member _godot_string should never be straight out overwritten ever without
first destroying the underlying string object's memory. This change solves the
problem through the introduction of a new private constructor to create String
objects with a pre-existing godot_string handle.