Hi Ævar
On 03/04/2022 14:57, Ævar Arnfjörð Bjarmason wrote:
[...]
av[++ac] = buf.buf;
av[++ac] = NULL;
setup_revisions(ac, av, &revs, NULL);
- revs.mailmap = &mailmap;
+ revs.mailmap = xmalloc(sizeof(struct string_list));
+ string_list_init_nodup(revs.mailmap);
This is a common pattern in one of the previous patches, is it worth
adding helpers to allocate and initialize a struct string_list? Maybe
string_list_new_nodup() and string_list_new_dup().
Maybe, but generally in the git codebase things malloc and then init(),
if we're going to add something like this *_new() that would be a change
for a lot more APIs than just mailmap.
And if it's just for mailmap I don't see how the inconsistency with
other code would be worth it.
It's for struct string_list not for mailmap and could be used for all
the conversions in patch 3. In general the split between allocation and
initialization is useful because it allows us to allocate structures on
the stack where possible or embed them in other structures. However if
there is a structure that is often allocated on the heap then I don't
think there is anything wrong with having a combined
allocate-and-initialize helper function which makes the code shorter and
eliminates the possibility of passing the wrong size to malloc().
diff --git a/revision.c b/revision.c
index 553f7de8250..622f0faecc4 100644
--- a/revision.c
+++ b/revision.c
@@ -2926,10 +2926,19 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
return left;
}
+static void release_revisions_mailmap(struct string_list
*mailmap)
+{
+ if (!mailmap)
+ return;
+ clear_mailmap(mailmap);
+ free(mailmap);
+}
It's not a big issue but if there are no other users of this then it
could just go inside release_revisions, my impression is that this
series builds a collection of very small functions whose only caller
is release_revisions()
Yes, these are just trivial static helpers so that each line in
release_revisions() corresponds to a member of the struct, without
loops, indentation for "don't free this" etc.
Fair enough
To the machine code it makes no difference at higher optimization
levels.
Indeed, not that these functions are particularly performance sensitive
in the first place.
Best Wishes
Phillip
