Re: pr 11067 patch

Joel Brobecker
2010-02-11T09:30:08Z

> 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