This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFC/RFA] add struct parse_context to all command functions
- From: "Ulrich Weigand" <uweigand at de dot ibm dot com>
- To: tromey at redhat dot com
- Cc: brobecker at adacore dot com (Joel Brobecker), gdb-patches at sourceware dot org
- Date: Wed, 22 Oct 2008 20:02:07 +0200 (CEST)
- Subject: Re: [RFC/RFA] add struct parse_context to all command functions
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