PR11067 patch

PR11067 patch

[Chris Moller]

[-- Attachment #1: Type: text/plain, Size: 1229 bytes --]

The attached patch fixes bug 11067 "p <enum constant> should print the 
constant's value" by providing a means of setting a format string to be 
used in printing enums.  The string is set with

    set enum-fmt <string>

show enum-fmt returns the current format string.

Here's the description from the relevant add_setshow_string_cmd()

    (gdb) help set enum-fmt
    Set enum display format.
    Allows a printf-style format string to be specified to control the
    display
    of enum values.  The string will be printed verbatim when enum
    values are
    displayed except that:
         %v directives will be expanded to the numeric value of the enum,
         %s directives will be expanded to the symbolic value of the
    enum, and
         %t directives will be expanded to the enum tag.
    The default format is equivalent to "%s" and the default will be used if
    no argument is provided to the set operation.
    (gdb)

This allows the user to print the value in any useful way.

    $1 = Val1
    $2 = Val1=(enum E)56
    $3 = Val2=(enum E)57
    $4 = Val1 = (enum E)56
    $5 = Val2 = (enum E)57
    $6 = 56 "Val1"
    $7 = 57 "Val2"

or whatever:

Patch attached, as is a diff of before-and-after gdb.sum



[-- Attachment #2: pr11067.patch --]
[-- Type: text/plain, Size: 8697 bytes --]

Index: gdb/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/ChangeLog,v
retrieving revision 1.11324
diff -u -r1.11324 ChangeLog
--- gdb/ChangeLog	4 Feb 2010 12:45:49 -0000	1.11324
+++ gdb/ChangeLog	6 Feb 2010 13:20:42 -0000
@@ -1,3 +1,9 @@
+Thu Feb  4 11:47:53 2010  Chris Moller  <moller@mollerware.com>
+
+	PR gdb/11067
+	* c-valprint.c (c_val_print _initialize_c_valprint):Provided
+	user-definable formatting for enum printing.
+
 2010-02-04  Tristan Gingold  <gingold@adacore.com>
 
 	* machoread.c (macho_add_oso): Renamed to macho_register_oso.
Index: gdb/c-valprint.c
===================================================================
RCS file: /cvs/src/src/gdb/c-valprint.c,v
retrieving revision 1.67
diff -u -r1.67 c-valprint.c
--- gdb/c-valprint.c	2 Feb 2010 16:45:16 -0000	1.67
+++ gdb/c-valprint.c	6 Feb 2010 13:20:43 -0000
@@ -30,8 +30,15 @@
 #include "c-lang.h"
 #include "cp-abi.h"
 #include "target.h"
+#include "command.h"
+#include "cli/cli-cmds.h"
 \f
 
+void _initialize_c_valprint (void);
+
+/* Enum format string.  */
+char *enum_format; 
+
 /* Print function pointer with inferior address ADDRESS onto stdio
    stream STREAM.  */
 
@@ -413,7 +420,96 @@
 	}
       if (i < len)
 	{
-	  fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
+	  if (enum_format && *enum_format)
+	    {
+	      struct cleanup *cleanups;
+	      char *enum_name;
+	      char *fmt_ptr;
+	      char *fmt = NULL;
+	      int fmt_max = 0;
+	      int fmt_idx = 0;
+#define FMT_LEN_INCR 256
+
+	      void chk_alloc (int amt)
+	      {
+		if (fmt_max <=  fmt_idx + amt)
+		  {
+		    fmt_max += amt + FMT_LEN_INCR;
+		    fmt = xrealloc (fmt, fmt_max);
+		  }
+	      }
+
+	      void fmt_cat_char (char c)
+	      {
+		chk_alloc (1);
+		fmt[fmt_idx++] = c;
+		fmt[fmt_idx] = 0;
+	      }
+
+	      void fmt_cat_str (char  *s)
+	      {
+		chk_alloc (strlen (s));
+		memcpy (fmt + fmt_idx, s, strlen (s));
+		fmt_idx += strlen (s);
+		fmt[fmt_idx] = 0;
+	      }
+
+	      int fprintf_enum (void *stream, const char *format, ...)
+	      {
+		va_list args;
+		va_start (args, format);
+		vfprintf_filtered (stream, format, args);
+		va_end (args);
+		return 0;
+	      }
+
+	      fmt_max = FMT_LEN_INCR + strlen (enum_format);
+	      fmt_idx = 0;
+	      fmt = xmalloc (fmt_max);
+	      cleanups = make_cleanup (xfree, fmt);
+
+	      for (fmt_ptr = enum_format; *fmt_ptr; fmt_ptr++)
+		{
+		  if (*fmt_ptr == '%' && *(fmt_ptr+1))
+		    {
+		      switch (*++fmt_ptr)
+			{
+			case 'v':
+			  fmt_cat_str ("%1$lld");
+			  break;
+			case 's':
+			  fmt_cat_str ("%2$s");
+			  break;
+			case 't':
+			  fmt_cat_str ("%3$s");
+			  break;
+			default:
+			  fmt_cat_char (*fmt_ptr);
+			  break;
+			}
+		    }
+		  else
+		    {
+		      fmt_cat_char (*fmt_ptr);
+		    }
+		}
+	      
+	      if (TYPE_NAME (type)) enum_name = TYPE_NAME (type);
+	      else if (TYPE_TAG_NAME(type)) enum_name = TYPE_TAG_NAME(type);
+	      else enum_name = "<unknown>";
+
+	      fprintf_enum (stream,
+				fmt,
+				val,
+				TYPE_FIELD_NAME (type, i),
+				enum_name);
+
+	      do_cleanups (cleanups);
+	    }
+	  else
+	    {
+	      fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
+	    }
 	}
       else
 	{
@@ -715,3 +811,26 @@
 		    value_address (val),
 		    stream, 0, &opts, current_language);
 }
+
+void
+_initialize_c_valprint (void)
+{
+  
+  enum_format = NULL;
+  add_setshow_string_cmd ("enum-fmt", no_class,
+			  &enum_format,
+			  _("Set enum display format."),
+			  _("Show enum display format."),
+			  _("\
+Allows a printf-style format string to be specified to control the display\n\
+of enum values.  The string will be printed verbatim when enum values are\n\
+displayed except that:\n\
+    %v directives will be expanded to the numeric value of the enum,\n\
+    %s directives will be expanded to the symbolic value of the enum, and\n\
+    %t directives will be expanded to the enum tag.\n\
+The default format is equivalent to \"%s\" and the default will be used if\n\
+no argument is provided to the set operation."),
+			  NULL,
+			  NULL,
+			  &setlist, &showlist);
+}
Index: gdb/testsuite/ChangeLog
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/ChangeLog,v
retrieving revision 1.2120
diff -u -r1.2120 ChangeLog
--- gdb/testsuite/ChangeLog	2 Feb 2010 23:40:28 -0000	1.2120
+++ gdb/testsuite/ChangeLog	6 Feb 2010 13:21:02 -0000
@@ -1,3 +1,10 @@
+2010-02-03  Chris Moller  <cmoller@redhat.com>
+
+	PR gdb/11067
+	* gdb.base/pr11067.exp: New.
+	* gdb.base/pr11067.c: New.
+	* gdb.base/Makefile.in (EXECUTABLES): Added new testcase.
+
 2010-02-02  Tom Tromey  <tromey@redhat.com>
 
 	* gdb.cp/virtbase.exp: Add regression tests.
Index: gdb/testsuite/gdb.base/Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.base/Makefile.in,v
retrieving revision 1.5
diff -u -r1.5 Makefile.in
--- gdb/testsuite/gdb.base/Makefile.in	15 Sep 2009 03:30:08 -0000	1.5
+++ gdb/testsuite/gdb.base/Makefile.in	6 Feb 2010 13:21:02 -0000
@@ -12,7 +12,8 @@
 	scope section_command setshow setvar shmain sigall signals \
 	solib solib_sl so-impl-ld so-indr-cl \
 	step-line step-test structs structs2 \
-	twice-tmp varargs vforked-prog watchpoint whatis catch-syscall
+	twice-tmp varargs vforked-prog watchpoint whatis catch-syscall \
+	pr11067
 
 MISCELLANEOUS = coremmap.data ../foobar.baz \
 	shr1.sl shr2.sl solib_sl.sl solib1.sl solib2.sl
Index: gdb/testsuite/gdb.base/pr11067.c
===================================================================
RCS file: gdb/testsuite/gdb.base/pr11067.c
diff -N gdb/testsuite/gdb.base/pr11067.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.base/pr11067.c	6 Feb 2010 13:21:02 -0000
@@ -0,0 +1,27 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2010 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+enum E {
+  Val1 = 56,
+  Val2
+};
+
+int main() {
+  enum E e = Val1;
+  return e;    //marker 1
+}
+
Index: gdb/testsuite/gdb.base/pr11067.exp
===================================================================
RCS file: gdb/testsuite/gdb.base/pr11067.exp
diff -N gdb/testsuite/gdb.base/pr11067.exp
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gdb/testsuite/gdb.base/pr11067.exp	6 Feb 2010 13:21:02 -0000
@@ -0,0 +1,58 @@
+# Copyright 2010 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+if { [skip_cplus_tests] } { continue }
+
+load_lib "cp-support.exp"
+
+set testfile "pr11067"
+set srcfile ${testfile}.c
+set binfile ${objdir}/${subdir}/${testfile}
+
+if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {c++ debug}] != "" } {
+     untested pr11067.exp
+     return -1
+}
+
+gdb_exit
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}
+
+if ![runto_main] then {
+    perror "couldn't run to breakpoint"
+    continue
+}
+
+# set a breakpoint at the return stmt
+
+gdb_breakpoint [gdb_get_line_number "marker 1"]
+gdb_continue_to_breakpoint "marker 1"
+
+gdb_test "print e"  "Val1"
+
+gdb_test "set enum-fmt %s=(enum %t)%v" ""
+gdb_test "print e"  "Val1=\\(enum E\\)56"
+
+gdb_test "set enum-fmt %s = (enum %t)%v" ""
+gdb_test "print Val2"  "Val2 = \\(enum E\\)57"
+
+gdb_test "show enum-fmt" "%s.* = \\(enum %t\\)%v.*"
+
+gdb_test "set enum-fmt" ""
+gdb_test "print e"  "Val1"
+
+gdb_exit
+return 0

[-- Attachment #3: pr11067-check.diff --]
[-- Type: text/plain, Size: 2838 bytes --]

--- gdb-virgin.sum	2010-02-04 11:57:38.079165155 -0500
+++ gdb-patched.sum	2010-02-06 08:21:38.035165293 -0500
@@ -1,4 +1,4 @@
-Test Run By moller on Thu Feb  4 11:35:04 2010
+Test Run By moller on Sat Feb  6 08:12:50 2010
 Native configuration is i686-pc-linux-gnu
 
 		=== gdb tests ===
@@ -5644,6 +5644,16 @@
 PASS: gdb.base/pr11022.exp: breakpoint hit 2
 PASS: gdb.base/pr11022.exp: set var x = 1
 PASS: gdb.base/pr11022.exp: watchpoint hit 2
+Running ../../../src/gdb/testsuite/gdb.base/pr11067.exp ...
+PASS: gdb.base/pr11067.exp: continue to breakpoint: marker 1
+PASS: gdb.base/pr11067.exp: print e
+PASS: gdb.base/pr11067.exp: set enum-fmt %s=(enum %t)%v
+PASS: gdb.base/pr11067.exp: print e
+PASS: gdb.base/pr11067.exp: set enum-fmt %s = (enum %t)%v
+PASS: gdb.base/pr11067.exp: print Val2
+PASS: gdb.base/pr11067.exp: show enum-fmt
+PASS: gdb.base/pr11067.exp: set enum-fmt
+PASS: gdb.base/pr11067.exp: print e
 Running ../../../src/gdb/testsuite/gdb.base/prelink.exp ...
 PASS: gdb.base/prelink.exp: prelink
 Running ../../../src/gdb/testsuite/gdb.base/printcmds.exp ...
@@ -14741,7 +14751,7 @@
 PASS: gdb.server/server-mon.exp: monitor set remote-debug 0
 Running ../../../src/gdb/testsuite/gdb.server/server-run.exp ...
 PASS: gdb.server/server-run.exp: loaded dynamic linker
-FAIL: gdb.server/server-run.exp: continue to main
+PASS: gdb.server/server-run.exp: continue to main
 Running ../../../src/gdb/testsuite/gdb.stabs/exclfwd.exp ...
 PASS: gdb.stabs/exclfwd.exp: ptype v1
 PASS: gdb.stabs/exclfwd.exp: ptype v2
@@ -15496,7 +15506,7 @@
 PASS: gdb.threads/watchthreads.exp: successfully compiled posix threads test case
 PASS: gdb.threads/watchthreads.exp: watch args[0]
 PASS: gdb.threads/watchthreads.exp: watch args[1]
-PASS: gdb.threads/watchthreads.exp: disable 3
+PASS: gdb.threads/watchthreads.exp: disable 2
 PASS: gdb.threads/watchthreads.exp: threaded watch loop
 PASS: gdb.threads/watchthreads.exp: first watchpoint on args[0] hit
 PASS: gdb.threads/watchthreads.exp: first watchpoint on args[1] hit
@@ -15509,7 +15519,7 @@
 PASS: gdb.threads/watchthreads2.exp: all threads started
 PASS: gdb.threads/watchthreads2.exp: watch x
 PASS: gdb.threads/watchthreads2.exp: set var test_ready = 1
-KFAIL: gdb.threads/watchthreads2.exp: gdb can drop watchpoints in multithreaded app (PRMS: gdb/10116)
+PASS: gdb.threads/watchthreads2.exp: all threads incremented x
 Running ../../../src/gdb/testsuite/gdb.trace/actions.exp ...
 PASS: gdb.trace/actions.exp: 5.1a: set three tracepoints, no actions
 PASS: gdb.trace/actions.exp: 5.1b: set actions for first tracepoint
@@ -15700,8 +15710,8 @@
 
 		=== gdb Summary ===
 
-# of expected passes		14896
-# of unexpected failures	23
+# of expected passes		14907
+# of unexpected failures	22
 # of expected failures		41
 # of untested testcases		3
 # of unsupported tests		63

Re: PR11067 patch

[Eli Zaretskii]

> Date: Sat, 06 Feb 2010 08:37:39 -0500
> From: Chris Moller <cmoller@redhat.com>
> 
> The attached patch fixes bug 11067 "p <enum constant> should print the 
> constant's value" by providing a means of setting a format string to be 
> used in printing enums.  The string is set with
> 
>     set enum-fmt <string>
> 
> show enum-fmt returns the current format string.

Thanks.  If this is accepted, we will need a suitable patch for the
manual, and also a NEWS entry.

Re: PR11067 patch

[Tom Tromey]

>>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:

Chris> The attached patch fixes bug 11067 "p <enum constant> should print the
Chris> constant's value" by providing a means of setting a format string to
Chris> be used in printing enums.  The string is set with

Chris>    set enum-fmt <string>

I would rather not introduce a new option for this, particularly a
formatting option.  We don't have this sort of thing elsewhere in gdb --
we just pick a printing format or two.

I re-read the thread on the archer list.  I propose having it print
like:

$2 = ENUMERATOR = (enum tag) 23

However, i would suppress the extra stuff in structs and when printing
in summary mode.

Tom

Re: PR11067 patch

[Chris Moller]

On 02/08/10 16:54, Tom Tromey wrote:
>>>>>> "Chris" == Chris Moller<cmoller@redhat.com>  writes:
>>>>>>              
>
> Chris>  The attached patch fixes bug 11067 "p<enum constant>  should print the
> Chris>  constant's value" by providing a means of setting a format string to
> Chris>  be used in printing enums.  The string is set with
>
> Chris>     set enum-fmt<string>
>
> I would rather not introduce a new option for this, particularly a
> formatting option.  We don't have this sort of thing elsewhere in gdb --
> we just pick a printing format or two.
>
> I re-read the thread on the archer list.  I propose having it print
> like:
>
> $2 = ENUMERATOR = (enum tag) 23
>
> However, i would suppress the extra stuff in structs and when printing
> in summary mode.
>    

Okay.  I'll have to figure out how to determine if I'm in structs (or, I 
suppose, unions, arrays, and any other high-data-volume circumstance 
that might occur.

cm

> Tom
>    

Re: PR11067 patch

[Pedro Alves]

On Monday 08 February 2010 21:54:22, Tom Tromey wrote:
> >>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:
> 
> Chris> The attached patch fixes bug 11067 "p <enum constant> should print the
> Chris> constant's value" by providing a means of setting a format string to
> Chris> be used in printing enums.  The string is set with
> 
> Chris>    set enum-fmt <string>
> 
> I would rather not introduce a new option for this, particularly a
> formatting option.  We don't have this sort of thing elsewhere in gdb --
> we just pick a printing format or two.

Ditto.

> I re-read the thread on the archer list.  I propose having it print
> like:
> 
> $2 = ENUMERATOR = (enum tag) 23
> 
> However, i would suppress the extra stuff in structs and when printing
> in summary mode.

I seriously don't want to start a bikeshed, but,
IMO, I don't think it's a good idea to to invent new
output syntax for this.  There's no need to print the type of
the enum, that's what ptype is for.  And we don't do it for any
other type.

We can look at enum's as having both a numeric facet, and
a symbolic facet, just like "char"s, and those we print as

(gdb) p 'a'
$1 = 97 'a'

If I had a choice, I would make printing enums exactly like:

 $2 = 23 'ENUMERATOR'

This is short, concise, and models existing output.  It
should look obvious what that output means.

And then users don't have to learn to recognize/identify
different outputs depending on where the type is
embedded or printed.  That sounds important usability
wise to me.

Has this option seriously been discarded?

-- 
Pedro Alves

Re: PR11067 patch

[Chris Moller]

On 02/08/10 17:32, Pedro Alves wrote:
> On Monday 08 February 2010 21:54:22, Tom Tromey wrote:
>    
>>>>>>> "Chris" == Chris Moller<cmoller@redhat.com>  writes:
>>>>>>>                
>> Chris>  The attached patch fixes bug 11067 "p<enum constant>  should print the
>> Chris>  constant's value" by providing a means of setting a format string to
>> Chris>  be used in printing enums.  The string is set with
>>
>> Chris>     set enum-fmt<string>
>>
>> I would rather not introduce a new option for this, particularly a
>> formatting option.  We don't have this sort of thing elsewhere in gdb --
>> we just pick a printing format or two.
>>      
>
> Ditto.
>
>    
>> I re-read the thread on the archer list.  I propose having it print
>> like:
>>
>> $2 = ENUMERATOR = (enum tag) 23
>>
>> However, i would suppress the extra stuff in structs and when printing
>> in summary mode.
>>      
>
> I seriously don't want to start a bikeshed, but,
> IMO, I don't think it's a good idea to to invent new
> output syntax for this.  There's no need to print the type of
> the enum, that's what ptype is for.  And we don't do it for any
> other type.
>
> We can look at enum's as having both a numeric facet, and
> a symbolic facet, just like "char"s, and those we print as
>
> (gdb) p 'a'
> $1 = 97 'a'
>
> If I had a choice, I would make printing enums exactly like:
>
>   $2 = 23 'ENUMERATOR'
>
> This is short, concise, and models existing output.  It
> should look obvious what that output means.
>
> And then users don't have to learn to recognize/identify
> different outputs depending on where the type is
> embedded or printed.  That sounds important usability
> wise to me.
>
> Has this option seriously been discarded?
>
>    

As I said a few days ago, no matter what format I use, someone won't 
like it.  I also asked The World to send me their objections /then/ 
rather than wait until I'd spent a couple of weekend nights coding the 
patch.

Re: PR11067 patch

[Pedro Alves]

On Monday 08 February 2010 22:47:00, Chris Moller wrote:

> As I said a few days ago, no matter what format I use, someone won't 
> like it.  

When matters are approached with too much subjective slack,
consensus is never reached.  That's why I tried to
explain my points as objectively as possible.

> I also asked The World to send me their objections /then/  
> rather than wait until I'd spent a couple of weekend nights coding the 
> patch.

I can understand your frustration, and I'm really sorry
that you went through the trouble of writing that
patch.  :-(

-- 
Pedro Alves

Re: PR11067 patch

[Eli Zaretskii]

> From: Pedro Alves <pedro@codesourcery.com>
> Date: Mon, 8 Feb 2010 22:32:39 +0000
> Cc: Chris Moller <cmoller@redhat.com>
> 
> If I had a choice, I would make printing enums exactly like:
> 
>  $2 = 23 'ENUMERATOR'
> 
> This is short, concise, and models existing output.  It
> should look obvious what that output means.

There's also a possibility to print enums as we do with macros (where
macro expansion is supported).

Re: PR11067 patch

[Joel Brobecker]

> If I had a choice, I would make printing enums exactly like:
> 
>  $2 = 23 'ENUMERATOR'

FWIW, this format seems fine to me.

-- 
Joel

Re: PR11067 patch

[Chris Moller]

On 02/08/10 16:54, Tom Tromey wrote:
>>>>>> "Chris" == Chris Moller<cmoller@redhat.com>  writes:
>>>>>>              
>
> Chris>  The attached patch fixes bug 11067 "p<enum constant>  should print the
> Chris>  constant's value" by providing a means of setting a format string to
> Chris>  be used in printing enums.  The string is set with
>
> Chris>     set enum-fmt<string>
>
> I would rather not introduce a new option for this, particularly a
> formatting option.  We don't have this sort of thing elsewhere in gdb --
> we just pick a printing format or two.
>    

The argument still applies that, no matter which format(s) are defined 
for multifaceted data (in this case, numeric value, symbolic value, and 
enum tag), it's not going to meet everyone's needs--the discussion on 
this thread demonstrates that fairly well.  The patch I'm suggesting 
offers the flexibility to easily define, using a simple, printf-like 
format string, without the necessity of writing Python code or whatever, 
any format people find useful.

The patch I'm suggesting is completely transparent to people who don't 
use it.  The presence of the set enum-fmt operation can be utterly 
ignored, with no loss of existing gdb capability,  by people who don't 
need it, but at the same time offers flexibility to people who can make 
use of it.

The same mechanism, BTW, could be used for other presently fixed-format 
multifaceted data such as characters (including, possibly, multibyte, 
UTF-style or wchar_t, characters, if gdb supports that kind of thing, 
but I haven't looked in detail at this.).  The mechanism also, BTW, 
introduces very little additional overhead in printing values--the user 
specified format string is translated, once, when set, into a printf 
format string subsequently used by vfprintf_filtered.

> I re-read the thread on the archer list.  I propose having it print
> like:
>
> $2 = ENUMERATOR = (enum tag) 23
>
> However, i would suppress the extra stuff in structs and when printing
> in summary mode.
>    

It's certainly possible to provide for multiple formats that are 
selected based on context--the only thing I don't know just off hand is 
how to determine that context.

> Tom
>    

Re: PR11067 patch

[Tom Tromey]

>>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:

Chris> The argument still applies that, no matter which format(s) are defined
Chris> for multifaceted data (in this case, numeric value, symbolic value,
Chris> and enum tag), it's not going to meet everyone's needs--the discussion
Chris> on this thread demonstrates that fairly well.

GDB classically bikesheds over output formatting and command naming, but
in my view these are generally disagreement based on aesthetic
preference, not needs.  In the end someone usually relents, we pick a
style, and move on.

My reason for not wanting this, aside from lack of any precedent, is
based on my belief that the differences are aesthetic and not
functional.  That leads me to conclude that in this case the cost of
another gdb option is outweighed by the triviality of the issue.

Chris> It's certainly possible to provide for multiple formats that are
Chris> selected based on context--the only thing I don't know just off hand
Chris> is how to determine that context.

Summary mode is a flag in value_print_options.  Other things can, I
think, be distinguished by recurse==0.

Tom

Re: PR11067 patch

[Chris Moller]

On 02/09/10 18:48, Tom Tromey wrote:
>
> My reason for not wanting this, aside from lack of any precedent, is
> based on my belief that the differences are aesthetic and not
> functional.  That leads me to conclude that in this case the cost

But what's the cost?  Functionally, it consists mostly of replacing an 
fputs_filtered with a vfprintf_filtered, so the performance hit is 
negligible, and the patch adds maybe a hundred lines of code, most of it 
in setting up the set enum-fmt command and translating the format strings.

> of another gdb option is outweighed by the triviality of the issue.
>    

Yep, so this is my last comment on the matter.

Re: PR11067 patch

[Joel Brobecker]

> But what's the cost?  Functionally, it consists mostly of replacing
> an fputs_filtered with a vfprintf_filtered, so the performance hit
> is negligible, and the patch adds maybe a hundred lines of code,
> most of it in setting up the set enum-fmt command and translating
> the format strings.

One lesson we (AdaCore) learnt from developping GPS (AdaCore's IDE), is
that adding options and flexibility can quickly cause the amount of testing
to explode.

I agree with Tom in this case that it's better to not have the formatting
option.  I know it can be discouraging sometimes that everyone bikesheds
a bit on things that small.  I actually think it's a good thing that
people provide their opinions, even at the risk of mild bikeshedding.
I'd rather have all opinions now rather than later.

But there comes a point when we need to make a decision, and I think
we have reached that point (or went past it? ;-) ).  I personally like
Pedro's suggestion, but I'm ready to accept any format at all. I don't
think anyone really expressed a strong disagreement, so I'd just review
the thread, select the format that seemed to be prefered based on feedback,
and just announce that this is what you'll implement.  I'm sure some will
feel like they must propose something that's obviously nicer than your
proposal, but if there are no objection, I suggest we go with what you
announced in the end.

-- 
Joel