Re: pr 11067 patch

Jan Kratochvil
2010-02-13T11:49:44Z

Hi Chris,

On Fri, 12 Feb 2010 16:48:13 +0100, Chris Moller wrote:
> --- gdb/testsuite/gdb.mi/mi-var-display.exp 1 Jan 2010 07:32:03 -0000 1.34
> +++ gdb/testsuite/gdb.mi/mi-var-display.exp 12 Feb 2010 15:38:07 -0000
> mi_gdb_test "-var-evaluate-expression anone" \
> - "\\^done,value=\"A\"" \
> + "\\^done,value=\"A = \\(enum <anonymous>\\)0\"" \
> "eval variable anone"

While it works with Eclipse should this format be used even for MI?
MI doc says:
returns its value as a string.

It is true it may be the same what `print' doc says:
By default, GDB prints a value according to its data type.

Still IMO the representation "A = (enum <anonymous>)0" is not suitable for MI
as it is not the intended type of a C expression.

But in practice for Eclipse it works and it brings better user experience - as
in CLI.

Some GNU Coding Standard nitpicks:

> Index: gdb/ChangeLog$
> +Wed Feb 10 17:13:44 2010 Chris Moller <moller@mollerware.com>

timestamp line does not follow GCS.

> + /*
> + When printing a simple enum value, the following code includes, in
> + addition to the symbolic value, the numeric value and the enum tag.
> + Under other-than-simple circumstances--in structs, arrays, etc.--
> + it does what's always done and prints just the symbolic value.
> + */

GCS comment should be formatted:
/* text...
text. */

> + else if (TYPE_TAG_NAME(type))
> + enum_name = TYPE_TAG_NAME(type);

GCS spacing: -> "TYPE_TAG_NAME (type)"

> + else
> + enum_name = "<anonymous>";
> +
= +^I $

git diff --check says:
gdb/c-valprint.c:433: trailing whitespace.

(but unaware how to check an already axisting .patch file)

> + fprintf_filtered (stream, "%s = (enum %s)%s",
->
# + fprintf_filtered (stream, "%s = (enum %s) %s",

while PR 11067 shows it that way GCS 5.3 suggests a whitespace there:
foo = (char *) malloc (sizeof *foo);

> +++ gdb/testsuite/gdb.base/pr11067.c 12 Feb 2010 15:38:05 -0000
> @@ -0,0 +1,48 @@
> +enum E {
> + Val1 = 56,
> + Val2
> +};

According to GNU indent it should be:

enum E
{
Val1 = 56,
Val2
};

> +struct Es {

ditto

> +int main() {

GCS:
int
main ()
{

> + /*
> + The following is a fake-out for linkers (such as, apparently, the AIX
> + linker) that optimises out unreference variables.
> + */

Comment -> GCS.

> + return 0;
> +}
> +

trailing whitespace line.

> +++ gdb/testsuite/gdb.base/pr11067.exp 12 Feb 2010 15:38:05 -0000
> +gdb_test "print e" "Val1 = \\(enum E\\)56"
> +
> +gdb_test "print ea" "{Val1, Val2, Val1}"
> +

Two spaces.

GDB testscases use more a practice to expect also leading " = ":
gdb_test "print e" " = Val1 = \\(enum E\\)56"
gdb_test "print ea" " = {Val1, Val2, Val1}"

> +gdb_test "print es" "v = 5, e = Val2}"

+ IMO missing opening bracket.

gdb_test "print es" " = {v = 5, e = Val2}"

Regards,
Jan