[RFA,1/3] Use function_name_style to print Ada and C function names

  gdb/ChangeLog
2019-01-10  Philippe Waroquiers  <philippe.waroquiers@skynet.be>

	* ada-typeprint.c (print_func_type): Print function name
	style to print function name.
	* c-typeprint.c (c_print_type_1): Likewise.
---
 gdb/ada-typeprint.c | 6 +++++-
 gdb/c-typeprint.c   | 8 ++++++--
 2 files changed, 11 insertions(+), 3 deletions(-)
  

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> 	* ada-typeprint.c (print_func_type): Print function name
Philippe> 	style to print function name.
Philippe> 	* c-typeprint.c (c_print_type_1): Likewise.

These are printing type names, not function names -- so I wonder whether
we want to apply the function name styling here.

Any comments on this aspect?  I am not sure myself.

On the one hand, it isn't a function name; maybe we would like a type
style, but then maybe a type style would be too noisy somehow?

On the other hand, maybe calling out function type names looks nicer.

Philippe> +	fputs_styled (varstring, function_name_style.style (), stream );

Extra space before the close paren.

Tom
  

On Sat, 2019-01-12 at 09:22 -0700, Tom Tromey wrote:
> > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> 	* ada-typeprint.c (print_func_type): Print function name
> Philippe> 	style to print function name.
> Philippe> 	* c-typeprint.c (c_print_type_1): Likewise.
> 
> These are printing type names, not function names -- so I wonder whether
> we want to apply the function name styling here.
> 
> Any comments on this aspect?  I am not sure myself.
ada-typeprint.c:print_func_type is only called to print types that are
functions.  So, that effectively prints a function name.

c-typeprint.c (c_print_type_1) is effectively called to print whatever
type (so not only functions), but the following added condition
should ensure function name style is only used for function names:
-      fputs_filtered (varstring, stream);
+      if (code == TYPE_CODE_FUNC || code == TYPE_CODE_METHOD)
+       fputs_styled (varstring, function_name_style.style (), stream );
+      else
+       fputs_filtered (varstring, stream);

> 
> On the one hand, it isn't a function name; maybe we would like a type
> style, but then maybe a type style would be too noisy somehow?
> 
> On the other hand, maybe calling out function type names looks nicer.
> 
> Philippe> +	fputs_styled (varstring, function_name_style.style (), stream );
> 
> Extra space before the close paren.
I will fix this, once/if a go to push ...

Thanks

Philippe
  

>>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:

Philippe> ada-typeprint.c:print_func_type is only called to print types that are
Philippe> functions.  So, that effectively prints a function name.

No opinion on that one, maybe Joel could say.

Philippe> c-typeprint.c (c_print_type_1) is effectively called to print whatever
Philippe> type (so not only functions), but the following added condition
Philippe> should ensure function name style is only used for function names:
Philippe> -      fputs_filtered (varstring, stream);
Philippe> +      if (code == TYPE_CODE_FUNC || code == TYPE_CODE_METHOD)
Philippe> +       fputs_styled (varstring, function_name_style.style (), stream );
Philippe> +      else
Philippe> +       fputs_filtered (varstring, stream);

Actually here I wonder if this can be hit.
Maybe
    typedef void Typename ();
... ?  But I'd sort of expect not.

Tom
  

> >>>>> "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Philippe> ada-typeprint.c:print_func_type is only called to print types that are
> Philippe> functions.  So, that effectively prints a function name.
> 
> No opinion on that one, maybe Joel could say.

I tried to think about the general context, and the guideline in
my mind is that there is a fine line where coloring is helpful and
when it becomes too much, reducing the effectiveness of colorization.
In this case, this patch seems to advocate in favor of colorization?

Perhaps we might indeed find it useful to colorize type names.
But, for the sake of consistency, let's color all type names,
rather than just function ones. And perhaps one thing we could do
to avoid an explosion of colorization options, which themselves
can lead to visual-confusion-by-color-mosaic effect, is to compromise
a bit by saying type names should be styled similarly to their object
counterparts.  Eg: same colour, but with a "dim" filter? For instance,
if addresses are in blue, then types which are pointers/refs, etc,
could be printed in dim blue?

One thing I might do, regardless of the decision, is define in the code
a different style for this kind of entity, even if, internally, the style
itself is just a reference (an "alias", if you will), of an existing
style. That way, it is easier to keep track of the various kinds of
entities and their style, thus allowing us the option of upgrading
this kind of entities to their own style if we wanted to.  Otherwise,
we'd have to audit all the existing calls to styling to see which
ones we want to style separately.
  

On Sun, 2019-01-13 at 09:18 +0400, Joel Brobecker wrote:
> > > > > > > "Philippe" == Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> > 
> > Philippe> ada-typeprint.c:print_func_type is only called to print types that are
> > Philippe> functions.  So, that effectively prints a function name.
> > 
> > No opinion on that one, maybe Joel could say.
> 
> I tried to think about the general context, and the guideline in
> my mind is that there is a fine line where coloring is helpful and
> when it becomes too much, reducing the effectiveness of colorization.
> In this case, this patch seems to advocate in favor of colorization?
> 
> Perhaps we might indeed find it useful to colorize type names.
The patch does not color type names.  It colors function names
that happens to be passed along with the type to the type
printing code.
See below  longer explanation.

> But, for the sake of consistency, let's color all type names,
> rather than just function ones. And perhaps one thing we could do
> to avoid an explosion of colorization options, which themselves
> can lead to visual-confusion-by-color-mosaic effect, is to compromise
> a bit by saying type names should be styled similarly to their object
> counterparts.  Eg: same colour, but with a "dim" filter? For instance,
> if addresses are in blue, then types which are pointers/refs, etc,
> could be printed in dim blue?
> 
> One thing I might do, regardless of the decision, is define in the code
> a different style for this kind of entity, even if, internally, the style
> itself is just a reference (an "alias", if you will), of an existing
> style. That way, it is easier to keep track of the various kinds of
> entities and their style, thus allowing us the option of upgrading
> this kind of entities to their own style if we wanted to.  Otherwise,
> we'd have to audit all the existing calls to styling to see which
> ones we want to style separately.
As I understand (and the trial I did confirms), the
code is really coloring *function names*, and is not coloring
*function type names*.
(I have tried to clarify this in the commit msg of the RFAv2).

Here is an example when debugging a piece of valgrind code
with the patched GDB.

Valgrind defines a function type HT_Cmp_t:
typedef Word  (*HT_Cmp_t) ( const void* node1, const void* node2 );
(a comparison function type, to be used in hash tables).

(gdb) info func HT_Cmp_t
All functions matching regular expression "HT_Cmp_t":
# this does not show anything, as expected, as
# the type HT_Cmp_t is not a function.

(gdb) info types HT_Cmp_t
All types matching regular expression "HT_Cmp_t":

File ../include/pub_tool_hashtable.h:
75:	typedef Word (*)(const void *, const void *) HT_Cmp_t;
# this shows the function type. The only part colored in
# the above is the filename.  In particular, the type name HT_Cmp_t
# is not colored (as expected, because the patch aims at coloring
# function names).

(gdb) info func cmp_pool_str
All functions matching regular expression "cmp_pool_str":

File m_deduppoolalloc.c:
205:	static Word cmp_pool_str(const void *, const void *);
# this shows a specific function of the type HT_Cmp_t
# and cmp_pool_str is colored (as expected) in function name style.

The comments in ada-typeprint.c are pretty clear (but I however cannot
replicate easily completely the above in Ada, as there is
no real notion of function type in Ada, or at least I could not
find it in the Ada Reference Manual : as I understand, the concept
of 'Ada function type' is a GDB concept.

/* Print function or procedure type TYPE on STREAM.  Make it a header
   for function or procedure NAME if NAME is not null.  */

static void
print_func_type (struct type *type, struct ui_file *stream, const char *name,
		 const struct type_print_options *flags)

So, print_func_type effectively prints a function type, and optionally
prints a function name given via the NAME arg.
The patch only colors NAME using function name style.

The c code is less clear documented than the ada print_func_type :
it uses varstring as argname.
The comments I found around the c and Ada type printing that references
VARSTRING in upper case are:
ada-typeprint.c:817:   If VARSTRING is a non-empty string, print as an Ada variable/field
typeprint.c:396:   variable named VARSTRING.  (VARSTRING is demangled if necessary.)

IMO, these comments are somewhat misleading, because
at that stage, VARSTRING is not necessarily a variable or a field name,
it can also be a function name (and maybe other things?).
So, VARSTRING might better be a more general NAME (or ENTITY_NAME, ...).

So, in summary:

I think the patch ensures type printing code is only coloring
function names (given as a 'side information' to the type to print)
using function name style, it is not coloring the type name,
that is probably somewhere deep in the type data structure.

Whether or not we want to style type names is another question
(and another patch then).

IMO, the patch looks reasonable, and produces a much nicer/more
readable output :).

Thanks

Philippe