From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5189 invoked by alias); 2 Jun 2010 22:58:07 -0000 Received: (qmail 5175 invoked by uid 22791); 2 Jun 2010 22:58:06 -0000 X-SWARE-Spam-Status: No, hits=-5.8 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_HI,SPF_HELO_PASS,TW_CP,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 02 Jun 2010 22:57:59 +0000 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o52Mvvla010312 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 2 Jun 2010 18:57:57 -0400 Received: from ns3.rdu.redhat.com (ns3.rdu.redhat.com [10.11.255.199]) by int-mx05.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id o52Mvuhu026127; Wed, 2 Jun 2010 18:57:56 -0400 Received: from opsy.redhat.com (ovpn01.gateway.prod.ext.phx2.redhat.com [10.5.9.1]) by ns3.rdu.redhat.com (8.13.8/8.13.8) with ESMTP id o52MvtNS019203; Wed, 2 Jun 2010 18:57:55 -0400 Received: by opsy.redhat.com (Postfix, from userid 500) id BBE253782DC; Wed, 2 Jun 2010 16:57:54 -0600 (MDT) From: Tom Tromey To: Chris Moller Cc: "gdb-patches\@sourceware.org" Subject: Re: [patch] pr10659 pretty-printing: Display vectors of vectors as matrix--rev 2 References: <4BFEA148.7000409@redhat.com> <4C0407E4.3020604@redhat.com> Reply-To: tromey@redhat.com Date: Wed, 02 Jun 2010 22:58:00 -0000 In-Reply-To: <4C0407E4.3020604@redhat.com> (Chris Moller's message of "Mon, 31 May 2010 15:03:00 -0400") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2010-06/txt/msg00088.txt.bz2 >>>>> "Chris" == Chris Moller 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