Re: [RFAv2 3/6] Add [-q] [-t TYPEREGEXP] [NAMEREGEXP] args to info [args|functions|locals|variables]

[Philippe Waroquiers <philippe.waroquiers@skynet.be>]

Feedback below ...

RFAv3 will follow soon

Thanks for the comments.

Philippe


On Tue, 2018-09-18 at 09:51 -0600, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe>   As stack.h now uses a type defined in gdb_regex.h, all files including stack.h
> Philippe>   need to include gdb_regex.h.
> 
> It's better to just include gdb_regex.h from stack.h in this situation.
Done (rather, no need anymore for regexp in stack.h).

> 
> Philippe> +	  if (preg
> 
> I think preg.has_value ().  gdb is trending away from these bool
> conversions, I think.  At least that's been the style for pointers.
> 
> Philippe> +	  if (treg
> 
> Here too.
Done.

> 
> Philippe> +/* Prepares the regular expression REG from REGEXP.
> Philippe> +   If REGEXP is NULL, it results in an empty regular expression.  */
> Philippe> +static void
> Philippe> +prepare_reg (const char *regexp, gdb::optional<compiled_regex> &reg)
> 
> Out parameters should be pointers in the gdb style.
Done.


> 
> Philippe> -  iterate_over_block_local_vars (block,
> Philippe> +  iterate_over_block_local_vars (block, preg, treg,
> Philippe>  				 do_print_variable_and_value,
> Philippe>  				 &cb_data);
> 
> iterate_over_block_local_vars already takes some opaque data that it
> just passes through to the callback.  So, I think there should not be
> a need to add the regexp parameters directly to it.  Instead they can be
> put into the cb_data here.
> 
> Philippe>  void
> Philippe>  iterate_over_block_arg_vars (const struct block *b,
> Philippe> +			     const gdb::optional<compiled_regex> &preg,
> Philippe> +			     const gdb::optional<compiled_regex> &treg,
> Philippe>  			     iterate_over_block_arg_local_vars_cb cb,
> Philippe>  			     void *cb_data)
> 
> Same here.

Done: the preg/treg regexp have been put in the opaque cb_data,
and regexp filtering logic has been moved to do_print_variable_and_value.


> 
> Philippe> +/* Returns true if the type_name of symbol_type of SYM matches TREG.
> Philippe> +   If SYM has no symbol_type or symbol_name, returns false.  */
> Philippe> +
> Philippe> +bool
> Philippe> +treg_matches_sym_type_name (const compiled_regex &treg,
> Philippe> +			    const struct symbol *sym)
> Philippe> +{
> Philippe> +  if (language_mode == language_mode_auto)
> Philippe> +    language_def (SYMBOL_LANGUAGE (sym))->la_print_type
> Philippe> +      (sym_type, "", &printed_sym_type_name,
> Philippe> +       -1, 0, &default_ptype_flags);
> 
> I think this won't actually work, because the type printing code is free
> to use current_language.  This is not good, but it is how it is.
> 
> So instead I think what this should do is save and restore the current
> language.  IIRC there's a helper RAII class for that already.
> 
> Then you can just call type_print and drop patch #2.
Done.

> 
> Philippe> +extern bool
> Philippe> +treg_matches_sym_type_name (const compiled_regex &treg,
> Philippe> +			    const struct symbol *sym);
>  
> No newline after the "bool" for a declaration.
Done.

Thanks
Philippe