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

[Chris Moller]

On 05/27/10 17:14, Tom Tromey wrote:
>>>>>> "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.
>    

I'm not altogether sure what you meant by this, but what I did was 
rename printers.py to pr10659.py and strip out everything but the 
vector/matrix code and tinker pr10659.exp to match.  This seems to be 
consistent with what py-prettyprint.* does--if I've guessed wrong, let 
me know.

(The real new printers.py has been moved to the right place in svn gcc 
and I'm creating the patch for that even as I type.)

> 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.
>    

done

> 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.
>    

A "matrix" hint turns on pretty mode--this seems to be consistent with 
what an "array" hint does.

> 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.
>    

done

> 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.
>    

done, using an alloca copy of the options

> 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?
>    

print_string_repr() is what prints the "std::vector of length..." stuff which I need to suppress for printing matrices, and matrix formatting needs is_py_none set to 1.


> 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.)
>    

see above--replaced by an alloca copy of options and a new matrix flag.
> Chris>  RCS file: testsuite/gdb.python/printers.py
> Chris>  diff -N testsuite/gdb.python/printers.py
> [...]
> Chris>  +print "in printers.py"
>
> Debugging code.
>    

done

> 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
>    

done, but it's part of this patch only as testcase code--the real thing 
is going into gcc.  also, the latest version of printers.py from svn gcc 
was a little different in th pretty_printers_dict stuff and in 
StdVectorPrinter.__init__.  my patch is with respect to that.

> Tom
>    

-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr10659.patch
Type: text/x-patch
Size: 13490 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20100531/e0b0f791/attachment.bin>