Improve concat_str C implementation#1350
Conversation
This is more optimal in some cases (especially appending to a string). It's very fiddly code though and I haven't tested it, so feel free to ignore this!
Test Results 13 files 28 suites 0s ⏱️ Results for commit 638d4e6. |
| if (str2_is_output) { | ||
| // Must memmove within stro because str2 has been invalidated | ||
| // and the source and destination may overlap. | ||
| memmove(*stro + in1_len, *stro, in2_len + 1); | ||
| } else { | ||
| in2 = (sail_string)str2; | ||
| // str2 is valid and a different string to stro so we can just copy. | ||
| memcpy(*stro + in1_len, str2, in2_len + 1); |
There was a problem hiding this comment.
memmove already does this kind of check so doing it here doesn't really add any efficiency. memcpy might even be an alias - I'd just unconditionally call memmove and keep the comment that they might overlap so we can't use memcpy
There was a problem hiding this comment.
Yep, but I made the mistake of unconditionally using memmove(*stro + in1_len, str2, in2_len + 1); in an earlier version and that was incorrect because the realloc may have freed str2. So the primary reason for the if() here is whether to copy from *stro or str2.
This is a perfect example of why Rust exists!
There was a problem hiding this comment.
That's a fair point I didn't see you were changing the source pointer.
|
I think we have a test case that is hitting this case, and they are all run with UBSan and address sanitiser in CI, so I'm pretty sure this is safe, and repeatedly appending is our most common use-case. |
3 participants
This is more optimal in some cases (especially appending to a string).
It's very fiddly code though and I haven't tested it, so feel free to ignore this!