@matheusbsbs / matheus-bagliano / commits / 7199203937

object_array: add and use `object_array_pop()`

In a couple of places, we pop objects off an object array `foo` by decreasing `foo.nr`. We access `foo.nr` in many places, but most if not all other times we do so read-only, e.g., as we iterate over the array. But when we change `foo.nr` behind the array's back, it feels a bit nasty and looks like it might leak memory. Leaks happen if the popped element has an allocated `name` or `path`. At the moment, that is not the case. Still, 1) the object array might gain more fields that want to be freed, 2) a code path where we pop might start using names or paths, 3) one of these code paths might be copied to somewhere where we do, and 4) using a dedicated function for popping is conceptually cleaner. Introduce and use `object_array_pop()` instead. Release memory in the new function. Document that popping an object leaves the associated elements in limbo. The converted places were identified by grepping for "\.nr\>" and looking for "--". Make the new function return NULL on an empty array. This is consistent with `pop_commit()` and allows the following: while ((o = object_array_pop(&foo)) != NULL) { // do something } But as noted above, we don't need to go out of our way to avoid reading `foo.nr`. This is probably more readable: while (foo.nr) { ... o = object_array_pop(&foo); // do something } The name of `object_array_pop()` does not quite align with `add_object_array()`. That is unfortunate. On the other hand, it matches `object_array_clear()`. Arguably it's `add_...` that is the odd one out, since it reads like it's used to "add" an "object array". For that reason, side with `object_array_clear()`. Signed-off-by: Martin Ågren <martin.agren@gmail.com> Reviewed-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Martin Ågren committed Sep 23, 2017 at 01:34 UTC 719920393737b3934a168f35ab45e09104edeed8

6 files changed +25 -10

builtin/fast-export.c

+1 -2

@@ -634,11 +634,10 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs)

634 {

635 struct commit *commit;

636 while (commits->nr) {

637 - commit = (struct commit *)commits->objects[commits->nr - 1].item;

637 + commit = (struct commit *)object_array_pop(commits);

638 if (has_unshown_parent(commit))

639 return;

640 handle_commit(commit, revs);

641 - commits->nr--;

641 }

642 }

643

builtin/fsck.c

+1 -6

@@ -181,12 +181,7 @@ static int traverse_reachable(void)

181 if (show_progress)

182 progress = start_progress_delay(_("Checking connectivity"), 0, 0, 2);

183 while (pending.nr) {

184 - struct object_array_entry *entry;

185 - struct object *obj;

186 -

187 - entry = pending.objects + --pending.nr;

188 - obj = entry->item;

189 - result |= traverse_one_object(obj);

184 + result |= traverse_one_object(object_array_pop(&pending));

185 display_progress(progress, ++nr);

186 }

187 stop_progress(&progress);

builtin/reflog.c

+1 -1

@@ -126,7 +126,7 @@ static int commit_is_complete(struct commit *commit)

126 struct commit *c;

127 struct commit_list *parent;

128

129 - c = (struct commit *)study.objects[--study.nr].item;

129 + c = (struct commit *)object_array_pop(&study);

130 if (!c->object.parsed && !parse_object(&c->object.oid))

131 c->object.flags |= INCOMPLETE;

132

object.c

+13

@@ -353,6 +353,19 @@ static void object_array_release_entry(struct object_array_entry *ent)

353 free(ent->path);

354 }

355

356 +struct object *object_array_pop(struct object_array *array)

357 +{

358 + struct object *ret;

359 +

360 + if (!array->nr)

361 + return NULL;

362 +

363 + ret = array->objects[array->nr - 1].item;

364 + object_array_release_entry(&array->objects[array->nr - 1]);

365 + array->nr--;

366 + return ret;

367 +}

368 +

369 void object_array_filter(struct object_array *array,

370 object_array_each_func_t want, void *cb_data)

371 {

object.h

@@ -116,6 +116,14 @@ int object_list_contains(struct object_list *list, struct object *obj);

116 void add_object_array(struct object *obj, const char *name, struct object_array *array);

117 void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);

118

119 +/*

120 + * Returns NULL if the array is empty. Otherwise, returns the last object

121 + * after removing its entry from the array. Other resources associated

122 + * with that object are left in an unspecified state and should not be

123 + * examined.

124 + */

125 +struct object *object_array_pop(struct object_array *array);

126 +

127 typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);

128

129 /*

shallow.c

+1 -1

@@ -99,7 +99,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,

99 cur_depth = 0;

100 } else {

101 commit = (struct commit *)

102 - stack.objects[--stack.nr].item;

102 + object_array_pop(&stack);

103 cur_depth = *(int *)commit->util;

104 }

105 }