This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: pr 11067 patch
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Chris Moller <cmoller at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Thu, 11 Feb 2010 13:29:50 +0400
- Subject: Re: pr 11067 patch
- References: <4B737180.4050802@redhat.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