Re: [PATCH 01/11] string_list: print_string_list to use trace_printf
- From
Stefan Beller <sbeller@google.com>
- Date
- Sep 6, 2018, 22:16 UTC
- Message-ID
- <CAGZ79kbRyVZ_1QpX0pPigN+2Pt2dV1eYvLyoWeSWejbLxko4YQ@mail.gmail.com>
- In-Reply-To
- <20180906165602.GA19367@sigill.intra.peff.net>
On Thu, Sep 6, 2018 at 9:56 AM Jeff King <peff@peff.net> wrote:
Show 36 quoted lines
>
> On Thu, Sep 06, 2018 at 09:52:28AM -0700, Junio C Hamano wrote:
>
> > Stefan Beller <sbeller@google.com> writes:
> >
> > > It is a debugging aid, so it should print to the debugging channel.
> >
> > ... and rename it with trace_ prefix.
> >
> > Use of trace_printf() is nice, as we can control its behavior at
> > runtime ;-)
>
> Yes, though...
>
> > > -void print_string_list(const struct string_list *p, const char *text)
> > > +void trace_print_string_list(const struct string_list *p, const char *text)
> > > {
> > > int i;
> > > if ( text )
> > > - printf("%s\n", text);
> > > + trace_printf("%s\n", text);
> > > for (i = 0; i < p->nr; i++)
> > > - printf("%s:%p\n", p->items[i].string, p->items[i].util);
> > > + trace_printf("%s:%p\n", p->items[i].string, p->items[i].util);
> > > }
>
> It seems funny that we'd iterate through the list checking over and over
> whether tracing is enabled.
>
> Should this do:
>
> if (!trace_want(&trace_default_key))
> return;
>
> at the top? (Or possibly even take a trace key from the caller, so that
> it can use whatever context makes sense for this particular list?)I added this check as well as rewording the commit message to recite Junios understanding of the patch as well.
However I would want to not derail this patch any further. This function was used as an aid by me some time ago, so I am willing to share the modifications needed for efficient printf debugging here, but I do not want to be dragged into a rabbit hole.
For example taking the trace key is much overkill IMHO for a pure debugging aid (and so is the shortcutting return that you proposed, but I added that already for the resend), so if anyone needs this function outside of printf-debugging, I would recommend patches on top.
Thanks, Stefan