Hey Junio,
On Sun, Aug 28, 2016 at 2:52 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Pranit Bauva <pranit.bauva@xxxxxxxxx> writes:
>
> >>> +struct bisect_terms {
> >>> + struct strbuf term_good;
> >>> + struct strbuf term_bad;
> >>> +};
> >>
> >> I think "struct strbuf" is overrated. ...
> >> I think you can just say "const char *" in this case.
> >
> > Using struct strbuf is not really overrated but in fact required. But
>
> Nothing is required.
>
> I can make your life easier by requiring you to never use struct
> strbuf as a structure field type, though. You will almost never
> need it unless you are building something exotic anyway.
>
> Step back and think what "strbuf" is good for. It holds a pointer
> to a piece of memory the field owns, over-allocates and knows the
> size of allocation and usage. That is good if you need to
>
> (1) frequently find out the length of the string; without a
> separate .len member you would have to run strlen().
>
> (2) incrementally add to the string in-place; as it overallocates,
> appending to the string would not have to involve realloc()
> every time and the cost of it is amortized.
>
> (3) make complex operations like splicing another string in,
> trimming substring out, etc.
>
> You need to do none of the above to these fields. term.term_good is
> either taken from an argv[] element, or you read from a line from a
> file and set it. You may do some trivial computation and set it to
> the result, like "the other field uses 'old', so this one need to be
> set to 'new'". The user of the field either has the string and sets
> it there, or reads the field's value as a whole string. No string
> manipulation in the field in-place is needed.
>
> > yes, for this patch it might seem as overrated. In the shell code
> > initally TERM_GOOD is set to "good" while TERM_BAD is set to "bad".
> > Now there are a lot of instances (one of which is bisect_start()
> > function) where this can change. So if we keep it as "const char *",
> > it would be right to change the value of it after wards. And we cannot
> > keep it as "char []" because we don't know its size before hand.
>
> You are not making sense. Nobody would suggest to use a fixed-size
> char array in this structure. That wouldn't even work in the case
> where you are stuffing what comes in an argv[] element in there,
> e.g.
>
> terms.term_good = argv[3];
>
> And you can of course support changing the value of the field
> without using strbuf. Just update the pointer to point at the piece
> of memory that holds the new value.
>
> In short, I do not see any good reason why the term_good field has
> to be anything other than "char *term_good" or "const char *term_good".
>
> Now, what you need to consider choosing between the two depends on
> where these strings can come from. If they are known to be always
> unchanging between the time you set it til the end of the program
> (e.g. using an element in argv[]), you can just assign without
> making any copy and you can use "const char *term_good". All other
> cases, the structure needs to take the ownership, and you would need
> to make a copy if you don't have the ownership, e.g.
>
> terms.term_good = xstrdup(argv[3]);
>
> You may be reading from a file, a line at a time and you may have a
> line's content in a strbuf. You do not (yet) own the buffer after
> reading it, e.g.
>
> strbuf_getline(&buf, fp);
> terms.term_good = strbuf_detach(&buf, NULL);
>
> Of course, if you need to take ownership of the memory, you would
> need to free(3) it as needed, which means the pattern to set the
> field would become
>
> free(terms.term_good);
> terms.term_good = ... some new value ...;
>
> Using strbuf as a local variable is good. It gives a higher level
> of abstraction when you are actually performing string operations.
> In most applications, however, a field in a struct is where the
> result of a step of computation is kept, not a scratch-pad to
> perform steps of computation in. When you are ready to update the
> value of a field, you _have_ a completed string, and you can just
> use "char *" field to point at it. There is no need for strbuf in
> the field.
>
> Don't look at the data structure used in trailer.[ch] as a model; it
> is an example of a terribly bad implementation taste, a pattern that
> should not be followed. Print it, not read it and burn it as a good
> symbolic gesture.
Thanks for explaining when to use strbuf. I am convinced that the
thing I am aiming for can be done with the help of "const char *".
Though I will have to use strbuf in get_terms() and detach the string
buffer from there as you have mentioned previously.
Regards,
Pranit Bauva