From: Jeff King <peff@peff.net>
To: "Martin Ågren" <martin.agren@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v2 5/6] object_array: add and use `object_array_pop()`
Date: Sat, 23 Sep 2017 00:27:57 -0400 [thread overview]
Message-ID: <20170923042757.ozl4qnmrsnd64mfc@sigill.intra.peff.net> (raw)
In-Reply-To: <80eaae517f73f57137db6adfcaef2e8ce16576c1.1506120292.git.martin.agren@gmail.com>
On Sat, Sep 23, 2017 at 01:34:53AM +0200, Martin Ågren wrote:
> 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.
All good reasons, I think.
> 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 interface looks appropriate for all of the current cases. Though I
do suspect there's a bit of catch-22 here. If a caller _did_ care about
the "name" and "path" fields, then this pop function would be
inappropriate, because it returns only the object field.
So in the most general form, you'd want:
while (foo.nr) {
struct object_array_entry *e = object_array_pop(&foo);
... do stuff with e->name, etc ...
object_array_release_entry(e);
}
But that is certainly more cumbersome for these callers. I think we can
punt on that until it becomes necessary (which likely is never).
> 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
> }
Agreed that the latter is more readable (though I am also happy that the
pop function does something sensible for an empty array).
> 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()`.
Yes, we're dreadfully inconsistent here. I tend to prefer noun_verb()
when "noun" is a struct we're operating on. But we have quite a bit of
verb_noun(). I find that noun_verb() is a bit more discoverable (e.g.,
tab completion does something sensible), but I'm not sure if it's worth
trying to do a mass-conversion.
Perhaps it's something that should be mentioned in CodingGuidelines, if
it isn't already.
> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
> builtin/fast-export.c | 3 +--
> builtin/fsck.c | 7 +------
> builtin/reflog.c | 2 +-
> object.c | 13 +++++++++++++
> object.h | 7 +++++++
> shallow.c | 2 +-
> 6 files changed, 24 insertions(+), 10 deletions(-)
The patch itself looks good, with one tiny nit:
> diff --git a/object.h b/object.h
> index 0a419ba8d..b7629fe92 100644
> --- a/object.h
> +++ b/object.h
> @@ -115,6 +115,13 @@ int object_list_contains(struct object_list *list, struct object *obj);
> /* Object array handling .. */
> void add_object_array(struct object *obj, const char *name, struct object_array *array);
> void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);
> +/*
> + * Returns NULL if the array is empty. Otherwise, returns the last object
> + * after removing its entry from the array. Other resources associated
> + * with that object are left in an unspecified state and should not be
> + * examined.
> + */
> +struct object *object_array_pop(struct object_array *array);
I'm very happy to see a comment over the declaration here. But I think
it's a bit more readable if we put a blank line between the prior
function and the start of that comment.
-Peff
next prev parent reply other threads:[~2017-09-23 4:28 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-20 19:47 [PATCH] diff-lib: clear `pending` object-array in `index_differs_from()` Martin Ågren
2017-09-20 20:02 ` Jeff King
2017-09-21 3:56 ` Martin Ågren
2017-09-21 4:52 ` Jeff King
2017-09-22 23:34 ` [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes Martin Ågren
2017-09-22 23:34 ` [PATCH v2 1/6] builtin/commit: fix memory leak in `prepare_index()` Martin Ågren
2017-09-22 23:34 ` [PATCH v2 2/6] commit: fix memory leak in `reduce_heads()` Martin Ågren
2017-09-22 23:34 ` [PATCH v2 3/6] leak_pending: use `object_array_clear()`, not `free()` Martin Ågren
2017-09-23 3:47 ` Jeff King
2017-09-22 23:34 ` [PATCH v2 4/6] object_array: " Martin Ågren
2017-09-23 4:04 ` Jeff King
2017-09-23 9:41 ` Martin Ågren
2017-09-22 23:34 ` [PATCH v2 5/6] object_array: add and use `object_array_pop()` Martin Ågren
2017-09-23 4:27 ` Jeff King [this message]
2017-09-23 9:49 ` Martin Ågren
2017-09-23 15:47 ` Jeff King
2017-09-22 23:34 ` [PATCH v2 6/6] pack-bitmap[-write]: use `object_array_clear()`, don't leak Martin Ågren
2017-09-23 4:35 ` Jeff King
2017-09-23 4:37 ` [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes Jeff King
2017-09-23 9:54 ` Martin Ågren
2017-09-23 16:13 ` Jeff King
2017-09-23 16:38 ` Jeff King
2017-09-24 19:59 ` Martin Ågren
2017-09-25 16:08 ` Jeff King
2017-10-01 15:04 ` Martin Ågren
2017-09-24 7:01 ` Junio C Hamano
2017-09-24 20:00 ` Martin Ågren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170923042757.ozl4qnmrsnd64mfc@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=git@vger.kernel.org \
--cc=martin.agren@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).