Re: [PATCH] Add completer to commands 'target {core/tfile/exec}'

[Pedro Alves <palves at redhat dot com>]

Nice!

On 03/28/2013 09:25 AM, Yao Qi wrote:
> 2013-03-28  Yao Qi  <yao@codesourcery.com>
> 
> 	* corelow.c: Include "completer.h".
> 	(_initialize_corelow): Call add_target_with_completer with parameter
> 	'filename_completer'.

s/parameter/argument/

> 	* tracepoint.c: Likewise.
> 	* exec.c (_initialize_exec): Likewise.
> 	* target.c (add_target): Rename to ...
> 	(add_target_with_completer): ... it.  New.  Call set_cmd_completer if
> 	parameter completer is not NULL.

s/it./this./.  "New." looks stale here.  Did you mean "New parameter"?


> 	(add_target): New.
> 	* target.h: Include "command.h".
> 	(add_target_with_completer): Declare it.
> 


> -/* Add a possible target architecture to the list.  */
> +/* Add a possible target architecture T to the list and add a new

s/Add a/Add / (you've identified T, so no longer indefinite).

> +   command 'target T->to_shortname'.  Set COMPLETER for the command
> +   completer if it is not NULL.  */

I suggest:

  "If not NULL, set COMPLETER as that command's completer."

>  
>  void
> -add_target (struct target_ops *t)
> +add_target_with_completer (struct target_ops *t,
> +			   completer_ftype *completer)

...

>  /* Adds a command ALIAS for target T and marks it deprecated.  This is useful
>     for maintaining backwards compatibility when renaming targets.  */
>  
> diff --git a/gdb/testsuite/gdb.base/completion.exp b/gdb/testsuite/gdb.base/completion.exp
> index cddc548..df93733 100644
> --- a/gdb/testsuite/gdb.base/completion.exp
> +++ b/gdb/testsuite/gdb.base/completion.exp
> @@ -713,6 +713,13 @@ gdb_test "complete set gnutarget aut" "set gnutarget auto"
>  
>  gdb_test "complete set cp-abi aut" "set cp-abi auto"
>  
> +# Test commands 'target FOO' completion works well.

"Test that completion of 'target FOO' commands works well."

> +
> +foreach target_name { "core" "tfile" "exec" } {
> +    gdb_test "complete target ${target_name} ./gdb.base/completion" \
> +	"target ${target_name} ./gdb.base/completion\\.exp.*"
> +}

Otherwise OK.

Thanks,
-- 
Pedro Alves