I agree that a list of char* is enough for language_tags.
Thanks for your review and patch. I'll apply your patch and send v9.
On Wed, Jan 28, 2015 at 3:15 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> On Tue, Jan 27, 2015 at 3:34 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Yi EungJun <semtlenori@xxxxxxxxx> writes:
>>
>>> +
>>> + sprintf(q_format, ";q=0.%%0%dd", decimal_places);
>>> +
>>> + strbuf_addstr(buf, "Accept-Language: ");
>>> +
>>> + for(i = 0; i < num_langs; i++) {
>>> + if (i > 0)
>>> + strbuf_addstr(buf, ", ");
>>> +
>>> + strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));
>>
>> This is not wrong per-se, but it looks somewhat convoluted to me.
>> ...
>
> Actually, this is wrong, isn't it?
>
> strbuf_detach() removes the language_tags[i].buf from the strbuf,
> and the caller now owns that piece of memory. Then strbuf_addstr()
> appends a copy of that string to buf, and the piece of memory
> that was originally held by language_tags[i].buf is now lost forever.
>
> This is leaking.
>
>>> + /* free language tags */
>>> + for(i = 0; i < num_langs; i++) {
>>> + strbuf_release(&language_tags[i]);
>>> + }
>
> ... because this loop does not free memory for earlier parts of language_tags[].
>
>> I am wondering if using strbuf for each of the language_tags[] is
>> even necessary. How about doing it this way instead?
>
> And I think my counter-proposal does not leak (as it does not us strbuf for
> language_tags[] anymore).
>
>>
>> http.c | 22 +++++++++-------------
>> 1 file changed, 9 insertions(+), 13 deletions(-)
>>
>> diff --git a/http.c b/http.c
>> index 6111c6a..db591b3 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -1027,7 +1027,7 @@ static void write_accept_language(struct strbuf *buf)
>> const int MAX_DECIMAL_PLACES = 3;
>> const int MAX_LANGUAGE_TAGS = 1000;
>> const int MAX_ACCEPT_LANGUAGE_HEADER_SIZE = 4000;
>> - struct strbuf *language_tags = NULL;
>> + char **language_tags = NULL;
>> int num_langs = 0;
>> const char *s = get_preferred_languages();
>> int i;
>> @@ -1053,9 +1053,7 @@ static void write_accept_language(struct strbuf *buf)
>> if (tag.len) {
>> num_langs++;
>> REALLOC_ARRAY(language_tags, num_langs);
>> - strbuf_init(&language_tags[num_langs - 1], 0);
>> - strbuf_swap(&tag, &language_tags[num_langs - 1]);
>> -
>> + language_tags[num_langs - 1] = strbuf_detach(&tag, NULL);
>> if (num_langs >= MAX_LANGUAGE_TAGS - 1) /* -1 for '*' */
>> break;
>> }
>> @@ -1070,13 +1068,12 @@ static void write_accept_language(struct strbuf *buf)
>>
>> /* add '*' */
>> REALLOC_ARRAY(language_tags, num_langs + 1);
>> - strbuf_init(&language_tags[num_langs], 0);
>> - strbuf_addstr(&language_tags[num_langs++], "*");
>> + language_tags[num_langs++] = "*"; /* it's OK; this won't be freed */
>>
>> /* compute decimal_places */
>> for (max_q = 1, decimal_places = 0;
>> - max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;
>> - decimal_places++, max_q *= 10)
>> + max_q < num_langs && decimal_places <= MAX_DECIMAL_PLACES;
>> + decimal_places++, max_q *= 10)
>> ;
>>
>> sprintf(q_format, ";q=0.%%0%dd", decimal_places);
>> @@ -1087,7 +1084,7 @@ static void write_accept_language(struct strbuf *buf)
>> if (i > 0)
>> strbuf_addstr(buf, ", ");
>>
>> - strbuf_addstr(buf, strbuf_detach(&language_tags[i], NULL));
>> + strbuf_addstr(buf, language_tags[i]);
>>
>> if (i > 0)
>> strbuf_addf(buf, q_format, max_q - i);
>> @@ -1101,10 +1098,9 @@ static void write_accept_language(struct strbuf *buf)
>> }
>> }
>>
>> - /* free language tags */
>> - for(i = 0; i < num_langs; i++) {
>> - strbuf_release(&language_tags[i]);
>> - }
>> + /* free language tags -- last one is a static '*' */
>> + for(i = 0; i < num_langs - 1; i++)
>> + free(language_tags[i]);
>> free(language_tags);
>> }
>>
--
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