Re: [PATCH v8 6/6] GDB/testsuite: Expand for character string limiting options

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

On Mon, 16 Jan 2023, Simon Marchi wrote:

> >  # Test interaction of the number of print elements to print and the
> >  # repeat count, set to the default of 10.
> >  
> > -proc test_print_repeats_10 {} {
> > +proc test_print_repeats_10_one { setting } {
> 
> Please expand the doc to say what SETTING is.

 Done.

> > +clean_restart ${testfile}
> > +gdb_breakpoint ${srcfile}:[gdb_get_line_number "set breakpoint 1 here"]
> > +
> > +# Verify that "start" lands inside the right procedure.
> > +if { [gdb_start_cmd] < 0 } {
> > +    untested start
> > +    return -1
> > +}
> 
> Can you update this to use runto_main, mirroring what was done in this
> commit?
> 
> https://gitlab.com/gnutools/binutils-gdb/-/commit/4a6bdfb9baa27e29151c7e97ae2abbe902f53638

 Done.

> >  # Test the max_elements option for gdb.Value.format_string.
> > -proc_with_prefix test_max_elements {} {
> > +proc test_max_string_one { setting unlimited } {
> 
> The comment above should be expanded a bit.

 Done.

> > -    check_format_string "an_array" $opts
> > -    check_format_string "an_array_with_repetition" $opts
> > +    if { $setting == "elements"} {
> 
> Space before closing bracket (just for consistency with the opening
> one).

 Fixed, thanks.

> > -    check_format_string "an_array_with_repetition" "" \
> > -      "\\{1, 3 <repeats 12 times>...\\}"
> > +    if { $setting == "elements"} {
> > +      check_format_string "an_array_with_repetition" "" \
> > +	"\\{1, 3 <repeats 12 times>...\\}"
> > +    }
> > +  }
> > +}
> > +
> > +proc_with_prefix test_max_string {} {
> 
> Is the name (and the name of test_max_string_one) accurate?  Does it
> test only strings, or strings and arrays?  Just wondering if
> test_max_strings_and_arrays would be more accurate.

 Ultimately it verifies `gdb.Value.format_string', so I think the name is 
accurate enough, even though with a couple of test cases we do use it with 
non-string arrays.

> > +  foreach_with_prefix setting { "elements" "characters" } {
> 
> The foreach_with_prefix here is not really necessary, because all tests
> under test_max_string_one are under a proc_with_prefix that depends on
> the setting passed.  You can remove it if you want to make test names a
> bit shorter, but I'm fine with leaving it as well for extra clarity.

 I've left it for consistency with `test_max_string_one', which also uses 
redundant `with_test_prefix' invocations throughout, so we have e.g.:

PASS: gdb.python/py-format-string.exp: format_string: lang_c: test_all_common: test_max_string: setting=elements: max_elements=200: a_point_t with option max_elements=200

where both "setting=elements: " and "max_elements=200: " could be removed, 
because we have "with option max_elements=200" already.  I guess this 
could be done with a separate change if there's a value in it.

> > +    test_max_string_one $setting \
> > +      [string map {elements 0 characters 4294967295} $setting]
> 
> Can you put a comment here and/or in test_max_string_one's comment,
> explaining what the last parameter does?  It's not clear to me what is
> done.

 Fixed with the `test_max_string_one' description.

 I have also fixed the Ada test case to use `require allow_ada_tests', 
recently introduced, and updated the copyright years for new files 
throughout.

 I'm posting the final version of the patch committed, in a reply to
this message.  Thank you for your review.

  Maciej