From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21155 invoked by alias); 22 Oct 2008 18:03:00 -0000 Received: (qmail 21145 invoked by uid 22791); 22 Oct 2008 18:02:58 -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; Wed, 22 Oct 2008 18:02:15 +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 m9MI28Dj026548 for ; Wed, 22 Oct 2008 18:02:08 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 m9MI28Cu4161734 for ; Wed, 22 Oct 2008 20:02:08 +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 m9MI27n8024032 for ; Wed, 22 Oct 2008 20:02:08 +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 m9MI27P1024024; Wed, 22 Oct 2008 20:02:07 +0200 Message-Id: <200810221802.m9MI27P1024024@d12av02.megacenter.de.ibm.com> Received: by tuxmaker.boeblingen.de.ibm.com (sSMTP sendmail emulation); Wed, 22 Oct 2008 20:02:07 +0200 Subject: Re: [RFC/RFA] add struct parse_context to all command functions To: tromey@redhat.com Date: Wed, 22 Oct 2008 18:03:00 -0000 From: "Ulrich Weigand" Cc: brobecker@adacore.com (Joel Brobecker), gdb-patches@sourceware.org In-Reply-To: from "Tom Tromey" at Oct 21, 2008 01:32:01 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/msg00546.txt.bz2 Tom Tromey wrote: > I've been meaning to do this for a while, so I went ahead today and > updated my patch to apply to cvs trunk. Thanks! > Basically I moved all print-formatting globals into a structure. To > ensure I didn't miss anything in the value_print/val_print hierarchy, > I actually removed the globals everywhere and replaced remaining > references to them with references to user_print_options (where the > global state now resides). This looks reasonable to me. However, (some of) the places where you now have to reference members of user_print_options seems to indicate either that something is really weird (why should evaluate_subexp_standard *evaluate* an expression differently depending on "objectprint" ??), or that some routines maybe need to get print options passed as arguments (print_formatted? address printing? the breakpoint print routines?). > A couple spots needed to make their own print-options structure; here > I made a couple of convenience functions to copy the global structure > and modify it to suit. I wrote them to initialize an argument to > avoid any possible confusion about ownership (a previous patch had > get_raw_print_options, e.g., return a pointer to a static struct -- > but this could be confusing, or even wrong if there is any recursion). In fact, I'm wondering if it wouldn't be nicer to also have something like a get_default_print_options function instead of refering to the user_print_options global in the top-level printing routines ... > This patch, I believe, fixes a latent bug. print_command_1 may set > the global inspect_it, but the value is not reset on error. I.e., > this function is missing a cleanup. I found this by inspection; I > haven't tried testing this theory. However, with your code it would appear "inspect" is now a no-op: @@ -846,10 +844,10 @@ print_command_1 (char *exp, int inspect, int voidprint) struct value *val; struct format_data fmt; int cleanup = 0; + struct value_print_options opts; - /* Pass inspect flag to the rest of the print routines in a global - (sigh). */ - inspect_it = inspect; + opts = user_print_options; + opts.inspect_it = inspect; if (exp && *exp == '/') { @@ -909,7 +907,6 @@ print_command_1 (char *exp, int inspect, int voidprint) if (cleanup) do_cleanups (old_chain); - inspect_it = 0; /* Reset print routines to normal. */ } Note how "opts" is never used throughout print_command_1 ... > Let me know what you think. I'd like to get something along these > lines into gdb. If this looks reasonable, I'll write a ChangeLog > entry and send it through testing. Or, if you want changes, let me > know that. Except for the issues discussed above, this definitely looks reasonable to me. There's still two globals used in various places throughout the print routines: current_language and current_gdbarch. Ideally, these should be replaced by passing arguments as well ... I'm not sure if the value_print_options structure is the correct place to put language and gdbarch pointer, though. Do you have and opinions how to handle those? Bye, Ulrich -- Dr. Ulrich Weigand GNU Toolchain for Linux on System z and Cell BE Ulrich.Weigand@de.ibm.com