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

[Simon Sobisch <simonsobisch@gnu.org>]

Am 26.01.2022 um 16:48 schrieb Maciej W. Rozycki:
> 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.

Thanks for still coming back :-)

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

I see, so some of my questions are possibly also not up-tp-date-

>>>> 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.

That's something we can work with - let's check the implementations and 
options below for that.

> 
>>>> 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)
>>>

Proposal 1 - Should work, shouldn't it?

>   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 do you mean with that? If the default is whatever print elements is 
it should be compatible, shouldn't it?

Please elaborate. Also: is that option "still on the table"?

>   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'.

Proposal 2 - That's an interesting idea. We could also say that in a 
later GDB version the default will be changed to on.
The best thing here: it is easily possible to have a different default 
for print characters (possibly much higher than print elements) with 
still keeping backward compatibility.

> * 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.

Proposal 3 - also an interesting idea, a clean split.
I tend to this one.
> 
> * 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.

Proposal 4 - has "complicated in"  - let's drop it.

>   Thoughts?
> 
>    Maciej

Thanks again for working on this,
Simon