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

[Tom Tromey <tromey at redhat dot com>]

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

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

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

What I meant is that it is fragile for our test suite to rely on
internal details of std::vector.  Instead, our test suite ought to be as
independent of compilers and libraries as possible.  In this particular
case, there is no reason to rely on std::vector in the test case,
because it is not difficult to write some new test code and a new
printer to exercise the matrix code paths.  This is what is done
elsewhere in gdb.python.

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

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

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

There is some confusing terminology here, in that multiple things are
called "pretty printing".

I think as a rule we should only emit newlines when asked for, either by
"set print array" or "set print pretty".  That is what I meant by the
above.

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

Tom> What is the rationale for this change?

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

I see.  I suppose it is ok to suppress the inner ones, but it doesn't
seem correct to suppress the outermost.

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

Wrong formatting.  Also see above about the newline situation...

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

...e.g.

Chris> +      else fputs_filtered ("}", stream);

Formatting.

Chris> +      options_copy = alloca (sizeof (struct value_print_options));
Chris> +      memcpy (options_copy, options, sizeof (struct value_print_options));

Just introduce a new local and copy it unconditionally.

Chris> +  else options_copy = (struct value_print_options *)options;

As a rule, never cast away const.

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

Formatting.  Let me recommend reading through the GNU coding standards.
They have a decent section on how C code should be formatted.


Some other general notes..

As this is a general facility for matrix printing, I wonder how you
would handle situations like a 2-dimensional array.

A new printing hint requires an update to the manual.

Please look at how this impacts MI varobjs, if at all.

Tom