On Thu, Jun 12, 2014 at 3:31 PM, Jeremiah Mahler <jmmahler@xxxxxxxxx> wrote:
> On Thu, Jun 12, 2014 at 11:50:41AM -0700, Junio C Hamano wrote:
>> I am on the fence.
>>
>> I have this suspicion that the addition of strbuf_set() would *only*
>> help when the original written with reset-and-then-add sequence was
>> suboptimal to begin with, and it helps *only* how the code reads,
>> without correcting the fact that it is still doing unnecessary
>> "first set to a value to be discarded and then reset to set the
>> right value", sweeping the issue under the rug.
>>
> It is certainly possible that builtin/remote.c (PATCH 2/2) saw the most
> benefit from this operation because it is so badly designed. But this
> seems unlikely given the review process around here ;-)
>
> The one case where it would be doing extra work is when a strbuf_set
> replaces a strbuf_add that didn't previously have a strbuf_reset.
> strbuf_set is not appropriate for all cases, as I mentioned in the
> patch, but in some cases I think it makes it more readable. And in this
> case it would be doing a reset on an empty strbuf. Is avoiding this
> expense worth the reduction in readability?
>
> Also, as Eric Sunshine pointed out, being able to easily re-order
> operations can make the code easier to maintain.
>
>> Repeated reset-and-then-add on the same strbuf used to be something
>> that may indicate that the code is doing unnecessary work. Now,
>> repeated uses of strbuf_set on the same strbuf replaced that pattern
>> to be watched for to spot wasteful code paths.
>>
> If a reset followed by and add was a rare occurrence I would tend to
> agree more.
When composing my review of the builtin/remote.c changes, I wrote
something like this:
Although strbuf_set() does make the code a bit easier to read
when strbufs are repeatedly re-used, re-using a variable for
different purposes is generally considered poor programming
practice. It's likely that heavy re-use of strbufs has been
tolerated to avoid multiple heap allocations, but that may be a
case of premature (allocation) optimization, rather than good
programming. A different ("better") way to make the code more
readable and maintainable may be to ban re-use of strbufs for
different purposes.
But I deleted it before sending because it's a somewhat tangential
issue not introduced by your changes. However, I do see strbuf_set()
as a Band-Aid for the problem described above, rather than as a useful
feature on its own. If the practice of re-using strbufs (as a
premature optimization) ever becomes taboo, then strbuf_set() loses
its value.
>> I dunno...
>>
>> > Signed-off-by: Jeremiah Mahler <jmmahler@xxxxxxxxx>
>> > ---
>> > Documentation/technical/api-strbuf.txt | 18 ++++++++++++++++++
>> > strbuf.c | 21 +++++++++++++++++++++
>> > strbuf.h | 13 +++++++++++++
>> > 3 files changed, 52 insertions(+)
>> >
>> > diff --git a/Documentation/technical/api-strbuf.txt b/Documentation/technical/api-strbuf.txt
>> > index f9c06a7..ae9c9cc 100644
>> > --- a/Documentation/technical/api-strbuf.txt
>> > +++ b/Documentation/technical/api-strbuf.txt
>> > @@ -149,6 +149,24 @@ Functions
>> > than zero if the first buffer is found, respectively, to be less than,
>> > to match, or be greater than the second buffer.
>> > /*----- add data in your buffer -----*/
> ...
>> > static inline void strbuf_addch(struct strbuf *sb, int c)
>> > {
>
> --
> Jeremiah Mahler
> jmmahler@xxxxxxxxx
> http://github.com/jmahler
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html