On 01/10/2014 12:01 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@xxxxxxxxxxxx> writes:
>
>> As long as we're being pathologically stingy with mallocs, we might as
>> well do the math right and save 6 (!) bytes.
>>
>> Signed-off-by: Michael Haggerty <mhagger@xxxxxxxxxxxx>
>> ---
>> It is left to the reader to show how another 7 bytes could be saved
>> (11 bytes on a 64-bit architecture!)
>>
>> It probably wouldn't kill performance to use a string_list here
>> instead.
>>
>> refs.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index ef9cdea..63b3a71 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -3351,10 +3351,10 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
>> size_t total_len = 0;
>> size_t offset = 0;
>>
>> - /* the rule list is NULL terminated, count them first */
>> + /* the rule list is NUL terminated, count them first */
>
> I think this _is_ wrong; it talks about the NULL termination of the
> ref_rev_parse_rules[] array, not each string that is an element of
> the array being NUL terminated.
Yes, you're right. Thanks for catching my sloppiness. Would you mind
squashing the fix onto my patch?
> Output from "git grep -e refname_match -e ref_rev_parse_rules"
> suggests me that we actually could make ref_rev_parse_rules[] a
> file-scope static to refs.c, remove its NULL termination and convert
> all the iterators of the array to use ARRAY_SIZE() on it, after
> dropping the third parameter to refname_match(). That way, we do
> not have to count them first here.
>
> But that is obviously a separate topic.
>
>> for (nr_rules = 0; ref_rev_parse_rules[nr_rules]; nr_rules++)
>> - /* no +1 because strlen("%s") < strlen("%.*s") */
>> - total_len += strlen(ref_rev_parse_rules[nr_rules]);
>> + /* -2 for strlen("%.*s") - strlen("%s"); +1 for NUL */
>> + total_len += strlen(ref_rev_parse_rules[nr_rules]) - 2 + 1;
>>
>> scanf_fmts = xmalloc(nr_rules * sizeof(char *) + total_len);
The way the code is written now (e.g., as long as it is not converted to
use a string_list or something) needs this loop not only to count the
number of rules but also to compute the total_len of the string into
which will be written all of the scanf format strings.
As for removing the third argument of refname_match(): although all
callers pass it ref_ref_parse_rules, that array is sometimes passed to
the function via the alias "ref_fetch_rules". So I suppose somebody
wanted to leave the way open to make these two rule sets diverge (though
I don't know how likely that is to occur). If we discard the third
argument to refname_match(), then we loose the distinction.
Thanks for your feedback,
Michael
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
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