Re: [PATCH 2/2] gdb: add SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT

[Simon Marchi <simark@simark.ca>]

On 12/9/22 12:42, Tom Tromey wrote:
>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Andrew> After the previous commit converted symbol lookup debug to use the new
> Andrew> debug scheme, this commit adds SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT.
> 
> Andrew> The previous commit updated 'set debug symbol-lookup' to use the new
> Andrew> debug scheme, however SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT still does
> Andrew> not exist.  The reason for this is that 'set debug symbol-lookup'
> Andrew> controls an integer variable, rather than a bool, and, depending on
> Andrew> the value of this variable, different amounts of debug will be
> Andrew> produced.
> 
> Andrew> Currently the *_SCOPED_DEBUG_ENTER_EXIT mechanism will only work for
> Andrew> control variables of type bool, this is because the underlying
> Andrew> scoped_debug_start_end class can only handle variables of type bool.
> 
> Andrew> This commit templates scoped_debug_start_end so that the class can
> Andrew> accept either a 'bool &' or an invokable object, e.g. a lambda
> Andrew> function, or a function pointer.
> 
> Andrew> The existing scoped_debug_start_end and scoped_debug_enter_exit macros
> Andrew> in common-debug.h are updated to support scoped_debug_enter_exit being
> Andrew> templated, however, nothing outside of common-debug.h needs to change.
> 
> Andrew> I've then added SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT in symtab.h, and
> Andrew> added a couple of token uses in symtab.c.  I didn't want to add too
> Andrew> much in this first commit, this is really about updating
> Andrew> common-debug.h to support this new ability.
> 
> This seems fine.  I suppose inlining will make it not cost too much... ?
> 
> Andrew> +#define SYMBOL_LOOKUP_SCOPED_DEBUG_ENTER_EXIT \
> Andrew> +  scoped_debug_enter_exit ([&] () -> bool { \
> Andrew> +    return symbol_lookup_debug > 0; \
> Andrew> +  }, "symbol-lookup")
> 
> I wonder whether the [&] is needed here.  I couldn't see what it is
> capturing.

Same comment here.  Instead of a lambda, you could have single function:

static bool
symbol_lookup_debug_enabled ()
{
  return symbol_lookup_debug > 0;
}

And then you could use it in symbol_lookup_debug_printf too, to factor
the logic.  You could have symbol_lookup_debug_v_enabled too for
symmetry if you wanted.

Otherwise, it all LGTM:

Approved-By: Simon Marchi <simon.marchi@efficios.com>

Simon