Eric Sunshine [off-list ref] writes:
quoted
+ for (i = start; i < argc; i++) {
+ if (safe_create_reflog(argv[i], &err, 1)) {
+ error("could not create reflog %s: %s", argv[i],
+ err.buf);
+ status = 1;
+ strbuf_release(&err);
This feels a bit dirty.
Hmm, I do not share that feeling. I wouldn't be surprised if you
found a lot of existing codepaths that run _init() a strbuf once,
work on it and then _release() once a section of code is done with
it, reuse it for further (unrelated) processing, knowing that
_release() implicitly did _init() and the strbuf is ready to use
after that. I thought that was a very well established pattern.
While it's true that the current
implementation of strbuf_release() re-initializes the strbuf (so
you're not technically wrong by re-using it), that's an implementation
detail; and indeed, the strbuf_release() documentation says:
Release a string buffer and the memory it used. You should not
use the string buffer after using this function, unless you
initialize it again.
Hmph. Perhaps the doc is wrong? ;-)
Alternatives would be strbuf_reset() or declaring and releasing the
strbuf within the for-loop scope.
Because _reset() just rewinds the .len pointer without deallocating,
you would need an extra _release() before it goes out of scope. If
it is expected that the strbuf will be reused for a number of times,
the length of the string each iteration uses is similar, and you
will iterate the loop many times, "_reset() each time and _release()
to clean-up" pattern would save many calls to realloc/free.
On Tue, Jun 30, 2015 at 12:07 PM, Junio C Hamano [off-list ref] wrote:
Eric Sunshine [off-list ref] writes:
quoted
quoted
+ for (i = start; i < argc; i++) {
+ if (safe_create_reflog(argv[i], &err, 1)) {
+ error("could not create reflog %s: %s", argv[i],
+ err.buf);
+ status = 1;
+ strbuf_release(&err);
This feels a bit dirty.
Hmm, I do not share that feeling. I wouldn't be surprised if you
found a lot of existing codepaths that run _init() a strbuf once,
work on it and then _release() once a section of code is done with
it, reuse it for further (unrelated) processing, knowing that
_release() implicitly did _init() and the strbuf is ready to use
after that. I thought that was a very well established pattern.
That could the case, and I may not be familiar with code doing that.
I have, however, seen plenty of code which uses strbuf_reset() in the
way you describe.
quoted
While it's true that the current
implementation of strbuf_release() re-initializes the strbuf (so
you're not technically wrong by re-using it), that's an implementation
detail; and indeed, the strbuf_release() documentation says:
Release a string buffer and the memory it used. You should not
use the string buffer after using this function, unless you
initialize it again.
Hmph. Perhaps the doc is wrong? ;-)
Perhaps. I always interpreted the documentation as meaning that the
project reserved the right to change the underlying implementation.
quoted
Alternatives would be strbuf_reset() or declaring and releasing the
strbuf within the for-loop scope.
Because _reset() just rewinds the .len pointer without deallocating,
you would need an extra _release() before it goes out of scope. If
it is expected that the strbuf will be reused for a number of times,
the length of the string each iteration uses is similar, and you
will iterate the loop many times, "_reset() each time and _release()
to clean-up" pattern would save many calls to realloc/free.
Yep, that's why I suggested strbuf_reset() as an alternative (and
likely would have chosen it myself).