Michael Haggerty [off-list ref] writes:
quoted hunk
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 <redacted>
---
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.
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);
On 01/10/2014 12:01 AM, Junio C Hamano wrote:
Michael Haggerty [off-list ref] writes:
quoted
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 <redacted>
---
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.
quoted
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@alum.mit.edu
http://softwareswirl.blogspot.com/