Add concat function to ustring and Strutil #2478
Conversation
src/libutil/strutil.cpp
Outdated
| } | ||
| memcpy(buf, s.data(), sl); | ||
| memcpy(buf + sl, t.data(), tl); | ||
| return std::string(local_buf, len); |
There was a problem hiding this comment.
You want buf here, not local_buf
There was a problem hiding this comment.
And ahem it looks like I was the one who wrote the code incorrectly in the first place :(
There was a problem hiding this comment.
That will teach me an important lesson about cut and paste from untrusted sources...
There was a problem hiding this comment.
And I am similarly embarrassed about not only failing to notice it while reviewing your code, but failing TWICE MORE when putting the equivalent into OIIO, and then one final failure by actually making test cases, but that do not cover the "long string" case.
src/libutil/strutil.cpp
Outdated
| } | ||
| for (int i = 0; i < n; ++i) | ||
| memcpy(buf + i * sl, str.data(), sl); | ||
| return std::string(local_buf, len); |
There was a problem hiding this comment.
You are right I hate to tell you this, Chris, but... can you guess?
`concat(s,t)` is semantically equivalent to `sprintf("%s%s",s,t)`, but
concat carefully avoids copying any of the characters more than once,
and it performs at most one temporary allocation (typically it does
not allocate temp memory from the heap at all, if the sum of the
strings' lengths are not too long).
Added both a std::string-returning and a ustring-returning version.
Also reworked the implementation of `Strutil::repeat()` to similarly
perform at most one one temporary allocation (typically none) and
not perform any unnecessary copies.
Inspired by work Chris Kulla did in OSL.
|
Updated. "Untested code is buggy code" has proven itself once again. |
|
LGTM! |
…#2478) `concat(s,t)` is semantically equivalent to `sprintf("%s%s",s,t)`, but concat carefully avoids copying any of the characters more than once, and it performs at most one temporary allocation (typically it does not allocate temp memory from the heap at all, if the sum of the strings' lengths are not too long). Added both a std::string-returning and a ustring-returning version. Also reworked the implementation of `Strutil::repeat()` to similarly perform at most one one temporary allocation (typically none) and not perform any unnecessary copies. Inspired by work Chris Kulla did in OSL.
concat(s,t)is semantically equivalent tosprintf("%s%s",s,t), butconcat carefully avoids copying any of the characters more than once,
and it performs at most one temporary allocation (typically it does
not allocate temp memory from the heap at all, if the sum of the
strings' lengths are not too long).
Added both a std::string-returning and a ustring-returning version.
Also reworked the implementation of
Strutil::repeat()to similarlyperform at most one one temporary allocation (typically none) and
not perform any unnecessary copies.
Inspired by work Chris Kulla did in OSL.