Re: [PATCH v2] gdb: split array and string limiting options

["Maciej W. Rozycki" <macro@embecosm.com>]

Hi Simon,

 Your message wasn't directly cc-ed to me and I missed it previously in 
the flood of mailing-list traffic.  Sorry about this.

 Please note that I have posted v3 before your message already.

> > > I really like the general idea and would like to see this added, but I
> > > think the patch goes too far and will break both old and new usage.
> > 
> > I don't understand what "new usage" means here.
> 
> Thanks for checking, that was unclear. With "new usage" I've meant that if
> someone temporarily sets the new variable in an extension there is no way to
> get back. The "-1 magic value" I've suggested would also allow a temporary
> setting outside of "with print characters ".
> 
> That actually reminds me of a documentation question I had: Does the
> implementation actually return the amount of characters (especially when the
> char array which is printed from has multiple multi-byte characters with
> combining symbols)?

 It does, see the bottom of my message at: 
<https://sourceware.org/pipermail/gdb-patches/2021-December/184466.html>.

> > > In my opinion there should be only 1 new test for the new setting and
> > > the other should be left unchanged while still working, because then
> > > existing usages will still work, too.

 Agreed.

> > > To do so the new parameter cannot be directly used as it is in the patch
> > > but must be fowarded to a new function or duplicated.
> > > 
> > > As an example this would mean instead of :
> > > 
> > > --- src.orig/gdb/c-valprint.c
> > > +++ src/gdb/c-valprint.c
> > > @@ -271,7 +271,7 @@ c_value_print_array (struct value *val,
> > > 
> > >      for (temp_len = 0;
> > >   	   (temp_len < len
> > > -	    && temp_len < options->print_max
> > > +	    && temp_len < options->print_max_chars
> > >   	    && extract_unsigned_integer (valaddr + temp_len * eltlen,
> > >   					 eltlen, byte	_order) != 0);
> > >   		   ++temp_len)
> > > 
> > > have the following
> > > 
> > > 
> > > +   const int print_max_chars = (options->print_max_chars != -1) ?
> > > options->print_max_chars : options->print_max;
> > >      for (temp_len = 0;
> > >   	   (temp_len < len
> > > -	    && temp_len < options->print_max
> > > +	    && temp_len < print_max_chars
> > >   	    && extract_unsigned_integer (valaddr + temp_len * eltlen,
> > >   					 eltlen, byte_order) != 0);
> > >   		   ++temp_len)
> > 
> > What follows is just my thoughts on what you propose.  This work was
> > being done for a client of Embecosm's, and Maciej is now driving this
> > patch, so what he does is up to him.  That said...
> > 
> > I did consider this when I originally wrote the patch.  But decided
> > against it in the end.
> > 
> > I agree that maintaining backwards compatibility is important.
> 
> :-)

 If you want to keep backwards compatibility, then no particular value for 
`set print elements' can be used to drive `set print characters' I'm 
afraid.  Someone may have used it somewhere.

 What I can offer as possible alternative solutions are:

* As you proposed, but instead driven by an entirely separate knob such as 
  `set print characters-enable <on|off>'; if "off", which would also be 
  the default, then keeping the current semantics of `set print elements'.

* Leave `set print elements' alone and instead introduce a replacement
  say `set print array-elements' command for array elements in addition to 
  `set print characters' already have proposed.  The latter two commands 
  would adjust the respective settings each while the former command would 
  set them both, for compatibility.  It's not clear to me what `show print 
  elements' would have to report however, maybe the `print array-elements' 
  setting.

* Have `set print elements' adjust both limits till `set print characters' 
  or `show print characters' have been used, at which point further 
  requests would only adjust the array element limit.  This wouldn't pose 
  a problem with `show print elements', but the semantics would be 
  complicated.

 Thoughts?

  Maciej