Re: [RFC/RFA] add struct parse_context to all command functions

Ulrich Weigand
2008-10-22T18:03:00Z

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