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

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

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