From: Thiago Farina <tfransosi@gmail.com>
Date: Tue, 21 Dec 2010 17:40:56 GMT
Subject: Re: [PATCH] trace.c: mark file-local function static
Message-ID: <AANLkTikvV1chYnSJUMGGoHTFv8EaXuK5B7Vo71jVyTRy@mail.gmail.com>
In-Reply-To: <7vbp4f9gzh.fsf@alter.siamese.dyndns.org>

On Tue, Dec 21, 2010 at 3:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> This is pretty much a clean-up patch from my perspective. Do we really
>> need two paragraph explanation for marking a function static?
>
> I've already applied it, but I think it is much better to do this instead
> (on top of Vasyl' Vavrychuk's patch).
>
> A more interesting topic is why the try-to-free-pack-memory logic needs to
> be disabled in the first place.  3a09425 (Do not call release_pack_memory
> in malloc wrappers when GIT_TRACE is used, 2010-05-08) explains that it is
> to avoid a race on Windows, and it looks like a workaround not a solution
> ("can be called without locking"---"why aren't we locking then?").
>
> Not that it matters in the context of "trace", which is a debugging
> facility, that this is a workaround.
>
> -- >8 --
> Subject: set_try_to_free_routine(NULL) means "do nothing special"
>
> This way, the next caller that wants to disable our memory reclamation
> machinery does not have to define its own do_nothing() stub.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  trace.c   |    8 ++------
>  wrapper.c |    2 ++
>  2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/trace.c b/trace.c
> index 62586fa..0fb2a2c 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -25,10 +25,6 @@
>  #include "cache.h"
>  #include "quote.h"
>
> -static void do_nothing(size_t unused)
> -{
> -}
> -
>  /* Get a trace file descriptor from GIT_TRACE env variable. */
>  static int get_trace_fd(int *need_close)
>  {
> @@ -76,7 +72,7 @@ void trace_printf(const char *fmt, ...)
>        if (!fd)
>                return;
>
> -       set_try_to_free_routine(do_nothing);    /* is never reset */
> +       set_try_to_free_routine(NULL);  /* is never reset */
>        strbuf_init(&buf, 64);
>        va_start(ap, fmt);
>        len = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);
> @@ -108,7 +104,7 @@ void trace_argv_printf(const char **argv, const char *fmt, ...)
>        if (!fd)
>                return;
>
> -       set_try_to_free_routine(do_nothing);    /* is never reset */
> +       set_try_to_free_routine(NULL);  /* is never reset */
>        strbuf_init(&buf, 64);
>        va_start(ap, fmt);
>        len = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);
> diff --git a/wrapper.c b/wrapper.c
> index 4c1639f..8d7dd31 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -12,6 +12,8 @@ static void (*try_to_free_routine)(size_t size) = do_nothing;
>  try_to_free_t set_try_to_free_routine(try_to_free_t routine)
>  {
>        try_to_free_t old = try_to_free_routine;
> +       if (!routine)
> +               routine = do_nothing;

Maybe I'm missing something, or I'm confused (or I do not understand
what I'm reading), but how you are assign routine to do_nothing if you
have removed do_nothing above?