Re: pr 11067 patch

[Joel Brobecker <brobecker at adacore dot com>]

> Provides a little more info on enums for simple 'p <enum>' cases;
> keeps the old format for complex cases like structs and arrays:

I feel really bad about this, and I really apologize - I am just only
suddenly wondering why this is considered a good idea, was that discussed?
Please understand that this is not an objection, but I just had a look
at the PR, and I happen to disagree with the reporter. According to me,
he said:

  1. If I print 'e', GDB prints 'Val1' and that's OK.
  2. If I print 'Val1', GDB prints also prints 'Val1' and he says
     that, instead, GDB should print its numerical value.

I disagree on (2) because, if he wanted the numerical value, he should
have told GDB. For instance:

  (gdb) p GREEN
  $1 = GREEN
  (gdb) p /d GREEN
  $2 = 1

If people still think that this suggestion is a good one, I looked at
the patch (the least I could do)...

> +Wed Feb 10 17:13:44 2010  Chris Moller  <moller@mollerware.com>
> +
> +	PR gdb/11067
> +	* c-valprint.c (c_val_print): In case TYPE_CODE_ENUM, add code to
> +	print the numeric value of the enum and the enum tag for
> +	top-level, non-summary "print enum"s.



> +	if (options->summary || recurse != 0)
> +	  {
> +	    fputs_filtered (TYPE_FIELD_NAME (type, i), stream);
> +	  }

We do not use the curly braces in this case, when the block only contains
one statement.

> +	else
> +	  {
> +	    char *enum_name;
> +
> +	    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>";

Can you place each statement on their own line? I am guessing that
GNU indent will fix that anyway, and I also think that this is harder
to read.

Rather than "unknown", I wonder if we shouldn't be using "<anonymous>".

> +	    fprintf_filtered (stream, "%s = (enum %s)%lld",
> +			      TYPE_FIELD_NAME (type, i),
> +			      enum_name,
> +			      val);

Just a gotcha, here: val is a LONGEST, which is not necessarily
a long long. Use plongest to format your value into a string.

Also, would you mind adding a short comment explaining why you are
doing all this (we want to be concise if printing this enum as part
of a larger data structure or while in summary mode).

> +	* gdb.cp/classes.exp (multiple places):
> +	* gdb.cp/m-data.exp (multiple places):
> +	* gdb.cp/m-static.exp (multiple places):
> +	* gdb.cp/namespace.exp (multiple places):
> +	* gdb.mi/mi-var-display.exp (multiple places):
> +	* gdb.mi/mi2-var-display.exp (multiple places):
> +	* gdb.python/py-value.exp (multiple places):
> +	* gdb.base/setvar.exp (multiple places): Updated expects to new
> +	enum format.

Hmmm, I don't think you need the "multiple places".  IIRC, the GCS allow
you to just say:
        
        * gdb.base/setvar.exp: Update throughout to match new enum format.

> +enum E e = Val1;
> +
> +enum E ea[] = { Val1, Val2, Val1 };
> +
> +struct Es es = { 5, Val2 };
> +
> +int main() {
> +  return 0;
> +}

I can see some linkers optimizing away your global variables, causing
the testcase to fail... The AIX linker, for instance, does that.
You have a look at exprs.c, or grep for AIX in gdb.base/*.c for some
ideas on how to deceive the linker and prevent this unwanted optimization.

> +if { [skip_cplus_tests] } { continue }
> +
> +load_lib "cp-support.exp"

I don't think this is right, is it? Since this is a C test, I don't
understand why that would be needed...

> +if  { [gdb_compile "${srcdir}/${subdir}/${srcfile}" "${binfile}" executable {c++ debug}] != "" } {
> +     untested pr11067.exp
> +     return -1

Same here, you're doing a C++ build for a C program.

Have a look at http://sourceware.org/gdb/wiki/GDBTestcaseCookbook.
If I were you, I'd just do the following to build your program:

        set testfile template
        set srcfile ${testfile}.c
        if { [prepare_for_testing ${testfile}.exp ${testfile} ${srcfile}] } {
            return -1
        }

The above also includes the calls to gdb_exit/gdb_start/etc, so they
can also go.

> +# set a breakpoint at the return stmt

Superfluous comment :).

> +gdb_exit
> +return 0

Also superfluous...

-- 
Joel