From: Junio C Hamano <gitster@pobox.com>
To: しらいしななこ <nanako3@lavabit.com>
Cc: git@vger.kernel.org, Adam Simpkins <adam@adamsimpkins.net>
Subject: Re: [PATCH] graph.c: make many functions static
Date: Thu, 19 Jun 2008 12:16:11 -0700 [thread overview]
Message-ID: <7vhcbptev8.fsf@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <20080619082110.6117@nanako3.lavabit.com> (nanako3@lavabit.com's message of "Thu, 19 Jun 2008 08:21:10 +0900")
しらいしななこ <nanako3@lavabit.com> writes:
> These function are not used anywhere. Also removes graph_release()
> that is never called.
>
> Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
I CCed Adam, who is the primary author in this area.
> ---
> graph.c | 57 +++++++++++++++++++++++++++++++++++++++++++--------------
> graph.h | 40 ----------------------------------------
> 2 files changed, 43 insertions(+), 54 deletions(-)
>
> diff --git a/graph.c b/graph.c
> index e2633f8..5f82170 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -4,6 +4,43 @@
> #include "diff.h"
> #include "revision.h"
>
> +/* Internal API */
> + ...
> +static int graph_next_line(struct git_graph *graph, struct strbuf *sb);
> +static void graph_padding_line(struct git_graph *graph, struct strbuf *sb);
> +static void graph_show_strbuf(struct git_graph *graph, struct strbuf const *sb);
I think these are probably fine, not in the sense that nobody calls these
functions _right now_ but in the sense that I do not foresee a calling
sequence outside the graph.c internal that needs to call these directly,
instead of calling graph_show_*() functions that use these.
> @@ -180,14 +217,6 @@ struct git_graph *graph_init(struct rev_info *opt)
> return graph;
> }
>
> -void graph_release(struct git_graph *graph)
> -{
> - free(graph->columns);
> - free(graph->new_columns);
> - free(graph->mapping);
> - free(graph);
> -}
But I do not think this is right. The current lack of caller of this
clean-up function simply means the current users are leaking. I think
they are all of "set up rev_info, do a lengthy operation and exit" pattern
and clean-up immediately before exit is often omitted as unnecessary, but
if we had a clean-up function for the revision API that function would
call this one. I'd rather leave this in place, and let libification
minded people figure out the cleanest places and ways to make this
called.
Other three clean-ups looked Ok to me. Thanks.
next prev parent reply other threads:[~2008-06-19 19:17 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-06-18 23:21 [PATCH] graph.c: make many functions static しらいしななこ
2008-06-19 19:16 ` Junio C Hamano [this message]
2008-06-20 6:00 ` Adam Simpkins
2008-06-20 7:37 ` Junio C Hamano
2008-06-20 17:19 ` Adam Simpkins
-- strict thread matches above, loose matches on Subject: below --
2008-09-25 9:41 Nanako Shiraishi
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vhcbptev8.fsf@gitster.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=adam@adamsimpkins.net \
--cc=git@vger.kernel.org \
--cc=nanako3@lavabit.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.
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 NNTP newsgroup(s).