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

[Tom Tromey <tromey@redhat.com>]

>>>>> "Chris" == Chris Moller <cmoller@redhat.com> writes:

Chris> This patch adds the mechanism for gdb to respond to "matrix" hints
Chris> from printers.py.

Thanks.

Chris> (Just to exercise the testcase, the patch includes
Chris> a patched version of printers.py to src/gdb/testsuite/gdb.python.  It
Chris> can be removed once the patched printers.py is generally available.)

This is not ok.  This part of the test suite ought to be independent
from the C++ compiler in use.  Instead, write some new code (C is fine)
and an associated printer to exercise the new code in GDB.  There are
other examples of this in gdb.python.

Chris> Question:  pr10659.exp yields:
Chris>    +ERROR: no fileid for qcore
Chris>    +ERROR: Couldn't send python print 'test' to GDB.
Chris>    +UNRESOLVED: gdb.python/pr10659.exp: verify python support
Chris> The actual tests pass, but I haven't the slightest idea how to get rid
Chris> of those errors or even if it's necessary to do so.

skip_python_tests requires a running gdb, so it must come after
prepare_for_testing.

Chris> +  pretty = (is_array || is_matrix)
Chris> +    ? options->prettyprint_arrays : options->pretty;

Wrong formatting.

Chris> +  if (is_matrix && recurse == 0)
Chris> +    fputs_filtered ("\n", stream);

I suspect this is not correct in the !pretty case.
Maybe for !pretty the matrix stuff should just be disabled, I don't see
what it would add.

Chris> +      else if (is_matrix) ;	/* Force a do-nothing.  */

Formatting.  Plus it is better to use empty braces with a comment
between, to emphasize the emptiness.

Chris> +  static int is_matrix = 0;

There has to be a better way.  Static locals like this are generally
bad.

One idea would be a new field in value_print_options.  This may be
slightly difficult depending on how recursion is done in some cases.

Chris> -  is_py_none = print_string_repr (printer, hint, stream, recurse,
Chris> -				  options, language, gdbarch);
Chris> +  is_py_none = is_matrix ? 1 : print_string_repr (printer, hint, stream,
Chris> +						  recurse, options,
Chris> +						  language, gdbarch);

What is the rationale for this change?

Chris> +  if (recurse == 0) is_matrix = 0;

Formatting.  Note also that this approach is not error-safe.  You need a
cleanup.  (Though with the value_print_options approach you won't,
because you can make a new local copy.)

Chris> RCS file: testsuite/gdb.python/printers.py
Chris> diff -N testsuite/gdb.python/printers.py
[...]
Chris> +print "in printers.py"

Debugging code.

Chris> +    def display_hint(self):
Chris> +        itype0  = self.val.type.template_argument(0)
Chris> +        rc = 'array'
Chris> +        if itype0.tag:
Chris> +            if -1 != itype0.tag.find('vector'):

This is not a suitable test.  Instead extract the regular expression
from the recognizer and use that.  Ideally, stuff the compiled form in a
global and use it in both places:

Chris> +    pretty_printers_dict[re.compile('^std::vector<.*>$')] = StdVectorPrinter

Tom