Re: [PATCH v3 2/4] gdb: testsuite: add new gdb_attach to check "attach" command

[Pedro Alves <pedro@palves.net>]

On 2022-03-17 17:04, Simon Marchi via Gdb-patches wrote:

> On 2022-03-17 12:50, Pedro Alves wrote:

>> That looks odd to me -- it's not obvious that "pattern" is the option name.
>>
>> If we instead use parse_args, the option would be specified with "-pattern" (leading dash),
>> and we'd use the magic TCL args array, so we wouldn't need to wrap the options in a list.
>> Like so (on top of the whole series):
> 
> I used parse_options because back when doing the rnglists support in the
> DWARF assembler, I had used parse_args and Tom Tromey mentioned he
> didn't really like that use of `args`, because it didn't make it clear
> what positional arguments were expected..  Unlike what you've done
> below, it was something like:
> 
>   proc rnglists { args } {
>   	# call parse_args, and then use the remaining args as positional
> 	# arguments
>   }

OK.  I'd be fine with that pattern myself, FWIW, as I'd assume the real arguments would
be described in a comment, which you always have to look at anyhow, even if you have
positional arguments:

   # Usage: func FOO BAR [-opt OPT]
   #
   proc func { args } {


Maybe we could instead come up with a proc alternative that would
let you specify the options in the args list directly, like:

   proc_with_opts func { foo bar } { {opt1 "default1"}
                                     {opt2 "default2"} } {
     ...
  }

... and we'd make it support users passing options before or after
positional args.

Maybe that's going a bit too far, dunno.


> The way you did it here has the advantage that it's clear which
> positional arguments are expected.  The downside is that flag arguments
> can't be put before positional arguments (like you usually can with
> shell or some tcl / expect procs).  But I think that's an acceptable
> tradeoff, I'm fine with it.

Yeah, it's what we do with gdb_test_multiple too, for example, even
thought we don't use parse_args there for some reason.

 # gdb_test_multiple COMMAND MESSAGE [ -prompt PROMPT_REGEXP] [ -lbl ]
 #                   EXPECT_ARGUMENTS
 ...
 proc gdb_test_multiple { command message args } {
 ... 

> 
> Could we add a way to tell parse_args that unexpected arguments are not
> allowed, so it could throw the error itself (and therefore be re-used
> elsewhere)?

I'd guess we could.  I wouldn't make that a requirement for this patch, though.