Re: [patch] pr10659 pretty-printing: Display vectors of vectors as matrix

[Phil Muldoon <pmuldoon@redhat.com>]

On 05/27/2010 05:43 PM, Chris Moller wrote:
> This patch adds the mechanism for gdb to respond to "matrix" hints from
> printers.py.  (Just to exercise the testcase, the patch includes a
> patched version of printers.py to src/gdb/testsuite/gdb.python.  It can
> be removed once the patched printers.py is generally available.)

This is not an official review, I just have some questions.

> @@ -397,7 +397,7 @@
> 		struct ui_file *stream, int recurse,
>  		const struct value_print_options *options,
>  		const struct language_defn *language,
> -		int is_py_none)
> +		int is_py_none, int is_matrix)
>  {

I'm not sure why it is necessary to pass the hint in here.  All of the
other hints are parsed internally within the function?


> -         if (is_py_none)
> -           fputs_filtered ("{", stream);
> -         else
> -           fputs_filtered (" = {", stream);
> +	  if (is_matrix && recurse == 0)
> +	    print_spaces_filtered (2 + 2 * recurse, stream);
> +	  if (is_py_none)
> +	    {
> +	      if (is_matrix && strcmp (hint, "array"))
> +		{

I looked at this, and I'm not sure what this means.  Either a hint is
a matrix or an array? Is this trying to deal with children?  I looked
at the modified vector printer and it can return either "array" or
"matrix" depending on the template argument.  


> +gdb_breakpoint [gdb_get_line_number "break"]
> +gdb_continue_to_breakpoint "break"
> +
> +gdb_test "p test1" "vector of length 2, capacity 2 =.*"
> +gdb_test "p test2" "= $nl  {$nl    {.*"
> +gdb_test "p test3" "= $nl  {$nl    {$nl      {.*"
> +

I cannot deduce from this test what the actual content of a vector
printed with the matrix hint should look like beyond the expected
bracket structure.

To be really picky, I'd at least like to see an example of the output
in comments or a more comprehensive test.  I've really gotten into
the idea that tests should also serve as an example of usage somehow.


> Index: testsuite/gdb.python/printers.py
> ===================================================================
> RCS file: testsuite/gdb.python/printers.py
> diff -N testsuite/gdb.python/printers.py

This file cannot exist in the GDB testsuite. It is already hosted
in GCC. You should post any changes to the vector printer there.  
For your testcase, I would write the test so it does not rely on this
file.

Cheers,

Phil