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

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

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