From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19017 invoked by alias); 23 Oct 2008 19:50:02 -0000 Received: (qmail 18966 invoked by uid 22791); 23 Oct 2008 19:50:01 -0000 X-Spam-Check-By: sourceware.org Received: from mtagate2.de.ibm.com (HELO mtagate2.de.ibm.com) (195.212.17.162) by sourceware.org (qpsmtpd/0.31) with ESMTP; Thu, 23 Oct 2008 19:49:17 +0000 Received: from d12nrmr1607.megacenter.de.ibm.com (d12nrmr1607.megacenter.de.ibm.com [9.149.167.49]) by mtagate2.de.ibm.com (8.13.1/8.13.1) with ESMTP id m9NJnC2v025990 for ; Thu, 23 Oct 2008 19:49:12 GMT Received: from d12av02.megacenter.de.ibm.com (d12av02.megacenter.de.ibm.com [9.149.165.228]) by d12nrmr1607.megacenter.de.ibm.com (8.13.8/8.13.8/NCO v9.1) with ESMTP id m9NJnCGU4132996 for ; Thu, 23 Oct 2008 21:49:12 +0200 Received: from d12av02.megacenter.de.ibm.com (loopback [127.0.0.1]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m9NJnBRe013010 for ; Thu, 23 Oct 2008 21:49:12 +0200 Received: from tuxmaker.boeblingen.de.ibm.com (tuxmaker.boeblingen.de.ibm.com [9.152.85.9]) by d12av02.megacenter.de.ibm.com (8.12.11.20060308/8.12.11) with SMTP id m9NJnBUn013007; Thu, 23 Oct 2008 21:49:11 +0200 Message-Id: <200810231949.m9NJnBUn013007@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Thu, 23 Oct 2008 21:49:11 +0200 Subject: Re: [RFC/RFA] add struct parse_context to all command functions To: tromey@redhat.com Date: Thu, 23 Oct 2008 19:50:00 -0000 From: "Ulrich Weigand" Cc: brobecker@adacore.com (Joel Brobecker), gdb-patches@sourceware.org In-Reply-To: from "Tom Tromey" at Oct 22, 2008 06:59:31 PM X-Mailer: ELM [version 2.5 PL2] MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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: 2008-10/txt/msg00584.txt.bz2 Tom Tromey wrote: > I implemented this, and I fixed the other problems you mentioned. > The result is appended. I still haven't tested this or written a > ChangeLog entry. Thanks! > This version reduces the use of user_print_options to the absolute > minimum. I don't think this is notably cleaner, but the difference > isn't extreme. If we cared, we could make it mildly less ugly by > having get_user_print_options return its argument. > > I still have a version of the patch without get_user_print_options in > case you change your mind on this. I do feel this version is nicer; in particular because it allows for more flexible implementation of get_user_print_options later on. I can see that it looks a bit ugly to do get_user_print_options just to look at, say, the addrprint flag. But many of those uses might instead be cleaned up by refactoring like you descibe below ... My choice would be to just go with what you have now. > Doing this did point out a couple things: > > * As you mentioned, breakpoint-printing could probably use a small > refactoring to give value_print_options arguments to various > methods. I can do this and roll it into this patch if you want; > though my first reaction is to leave it as a follow-up patch (or > just not do it). Let me know what you want. I think that would be nice, but not really important -- this can certainly be done in a follow-up patch. > * print_scalar_formatted probably needs an options argument as well. > What do you think? This worries me a little since it is another 47 > call sites; so it could make the patch even bigger. Maybe this > ought to be a follow-up. This looks more critical, and could in fact even be a bug: for example, in the cases where print_scalar_formatted recurses back to val_print, you now lose the inspect_it setting ... > Ulrich> OK, I'm fine with this. However, this means that we'll really have > Ulrich> to construct the option struct on the fly, we cannot have a global > Ulrich> struct hold pre-initialized pointers to current_language etc. > > There's always "GCC style": > > #define current_language user_print_options.language > > Semi-awful. > > Or just s/current_language/user_print_options.current_language/ > everywhere. This, IMO, is not terrible, aside from its verbosity. As current_language (and even more so, current_gdbarch) is used in many places that have nothing whatsoever to do with printing, I don't think it is a good idea to do that. If we add language and gdbarch fields to value_print_options, I would expect those to be dynamically instantiated by the get_... functions; either from whatever global holds the "primary" value, or -more likely, as those globals really need to go away at some point- from arguments to the get_... routines. (Allowing for dynamic instantiation like this is one reason why I prefer always using get_... routines instead of just accessing the global struct.) > This is related to Joel's patch and to Daniel's question about a > policy for access to globals. Before going too far down this road it > may be worth coming up with an agreement there. With current_gdbarch and current_language, I think the only clean long-term solution is for these globals to go away completely. In the general case of a multi-arch multi-language inferior they just don't make sense. (The same holds IMO for some other pervasively used globals like current_target and inferior_ptid.) So the question is really how to handle the transition period until they are gone -- and my feeling would be that accesses should be pushed up the stack as far as possible, in general. > To sum up, once I know how much more I should do, I will implement it > and then submit the patch for real. I am not looking forward to > writing this ChangeLog entry... One other issue I noticed: the deref_ref flag seems to be handled incorrectly with your current patch. It is always 0 in the structs returned by the get_... routines, and never set to any nonzero value manually either, as far as I can see. Apart from that, the patch looked good to me. Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com