Re: pr 11067 patch

[Jan Kratochvil <jan.kratochvil@redhat.com>]

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