Add completer for skip numbers

Message ID 20181110173941.26165-1-simon.marchi@polymtl.ca
State New
Headers show
Series
  • Add completer for skip numbers
Related show

Commit Message

Simon Marchi Nov. 10, 2018, 5:39 p.m.
Add completer to various commands that accept skip numbers:

  - skip enable
  - skip disable
  - skip delete
  - info skip

These commands also accept ranges, but I am not too sure of how to do
that properly, so I went for the simpler goal of complete just numbers.

A future idea would be to make a re-usable and well-tested completer for
numbers and ranges.  I think it could at least be re-used for breakpoint
numbers (for example with the "enable breakpoints" command).

gdb/ChangeLog:

	* skip.c (complete_skip_number): New function.
	(_initialize_step_skip): Add completers to some skip commands.

gdb/testsuite/ChangeLog:

	* gdb.base/skip.exp: Add standard_testfile.  Add "skip delete"
	completer tests.
---
 gdb/skip.c                      | 35 +++++++++++++++++++-----
 gdb/testsuite/gdb.base/skip.exp | 48 +++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+), 7 deletions(-)

-- 
2.19.1

Comments

Pedro Alves Nov. 10, 2018, 7:07 p.m. | #1
On 11/10/2018 05:39 PM, Simon Marchi wrote:
> Add completer to various commands that accept skip numbers:

> 

>   - skip enable

>   - skip disable

>   - skip delete

>   - info skip

> 

> These commands also accept ranges, but I am not too sure of how to do

> that properly, so I went for the simpler goal of complete just numbers.

> 

> A future idea would be to make a re-usable and well-tested completer for

> numbers and ranges.  I think it could at least be re-used for breakpoint

> numbers (for example with the "enable breakpoints" command).


And threads.

> 

> gdb/ChangeLog:

> 

> 	* skip.c (complete_skip_number): New function.

> 	(_initialize_step_skip): Add completers to some skip commands.

> 

> gdb/testsuite/ChangeLog:

> 

> 	* gdb.base/skip.exp: Add standard_testfile.  Add "skip delete"

> 	completer tests.

> ---

>  gdb/skip.c                      | 35 +++++++++++++++++++-----

>  gdb/testsuite/gdb.base/skip.exp | 48 +++++++++++++++++++++++++++++++++

>  2 files changed, 76 insertions(+), 7 deletions(-)

> 

> diff --git a/gdb/skip.c b/gdb/skip.c

> index 13db0f6b43c6..f2a1df79e8bf 100644

> --- a/gdb/skip.c

> +++ b/gdb/skip.c

> @@ -641,6 +641,23 @@ function_name_is_marked_for_skip (const char *function_name,

>    return false;

>  }

>  

> +/* Completer for skip numbers.  */

> +

> +static void

> +complete_skip_number (cmd_list_element *cmd,

> +		      completion_tracker &completer,

> +		      const char *text, const char *word)

> +{

> +  size_t word_len = strlen (word);

> +

> +  for (const skiplist_entry &entry : skiplist_entries)

> +    {

> +      gdb::unique_xmalloc_ptr<char> name (xstrprintf ("%d", entry.number ()));

> +      if (strncmp (word, name.get (), word_len) == 0)

> +	completer.add_completion (std::move (name));

> +    }

> +}

> +

>  void

>  _initialize_step_skip (void)

>  {

> @@ -676,28 +693,31 @@ If no function name is given, skip the current function."),

>  	       &skiplist);

>    set_cmd_completer (c, location_completer);

>  

> -  add_cmd ("enable", class_breakpoint, skip_enable_command, _("\

> +  c = add_cmd ("enable", class_breakpoint, skip_enable_command, _("\

>  Enable skip entries.  You can specify numbers (e.g. \"skip enable 1 3\"), \

>  ranges (e.g. \"skip enable 4-8\"), or both (e.g. \"skip enable 1 3 4-8\").\n\n\

>  If you don't specify any numbers or ranges, we'll enable all skip entries.\n\n\

>  Usage: skip enable [NUMBER | RANGE]..."),

> -	   &skiplist);

> +	       &skiplist);

> +  set_cmd_completer (c, complete_skip_number);

>  

> -  add_cmd ("disable", class_breakpoint, skip_disable_command, _("\

> +  c = add_cmd ("disable", class_breakpoint, skip_disable_command, _("\

>  Disable skip entries.  You can specify numbers (e.g. \"skip disable 1 3\"), \

>  ranges (e.g. \"skip disable 4-8\"), or both (e.g. \"skip disable 1 3 4-8\").\n\n\

>  If you don't specify any numbers or ranges, we'll disable all skip entries.\n\n\

>  Usage: skip disable [NUMBER | RANGE]..."),

> -	   &skiplist);

> +	       &skiplist);

> +  set_cmd_completer (c, complete_skip_number);

>  

> -  add_cmd ("delete", class_breakpoint, skip_delete_command, _("\

> +  c = add_cmd ("delete", class_breakpoint, skip_delete_command, _("\

>  Delete skip entries.  You can specify numbers (e.g. \"skip delete 1 3\"), \

>  ranges (e.g. \"skip delete 4-8\"), or both (e.g. \"skip delete 1 3 4-8\").\n\n\

>  If you don't specify any numbers or ranges, we'll delete all skip entries.\n\n\

>  Usage: skip delete [NUMBER | RANGES]..."),

> -           &skiplist);

> +	       &skiplist);

> +  set_cmd_completer (c, complete_skip_number);

>  

> -  add_info ("skip", info_skip_command, _("\

> +  c = add_info ("skip", info_skip_command, _("\

>  Display the status of skips.  You can specify numbers (e.g. \"skip info 1 3\"), \

>  ranges (e.g. \"skip info 4-8\"), or both (e.g. \"skip info 1 3 4-8\").\n\n\

>  If you don't specify any numbers or ranges, we'll show all skips.\n\n\

> @@ -705,6 +725,7 @@ Usage: skip info [NUMBER | RANGES]...\n\

>  The \"Type\" column indicates one of:\n\

>  \tfile        - ignored file\n\

>  \tfunction    - ignored function"));

> +  set_cmd_completer (c, complete_skip_number);

>  

>    add_setshow_boolean_cmd ("skip", class_maintenance,

>  			   &debug_skip, _("\

> diff --git a/gdb/testsuite/gdb.base/skip.exp b/gdb/testsuite/gdb.base/skip.exp

> index 223c93d0d9b6..ee328bf35c9b 100644

> --- a/gdb/testsuite/gdb.base/skip.exp

> +++ b/gdb/testsuite/gdb.base/skip.exp

> @@ -16,6 +16,8 @@

>  # This file was written by Justin Lebar. (justin.lebar@gmail.com)

>  # And further hacked on by Doug Evans. (dje@google.com)

>  

> +standard_testfile

> +

>  if { [prepare_for_testing "failed to prepare" "skip" \

>                            {skip.c skip1.c } \

>                            {debug nowarnings}] } {

> @@ -297,3 +299,49 @@ with_test_prefix "step using -fi + -fu" {

>      gdb_test "step" ".*" "step 4"; # Skip over test_skip()

>      gdb_test "step" "test_skip_file_and_function \\(\\) at.*" "step 5"; # Return from skip1_test_skip_file_and_function()

>  }

> +

> +with_test_prefix "skip delete completion" {

> +    global binfile

> +    clean_restart "${binfile}"

> +    if ![runto_main] {

> +	fail "can't run to main"

> +	return

> +    }

> +

> +    # Create a bunch of skips, don't care what they are.

> +    for {set i 0} {$i < 12} {incr i} {

> +	gdb_test "skip" ".*" "add skip $i"

> +    }

> +

> +    gdb_test "complete skip delete " \

> +	[multi_line "skip delete 1" \

> +		    "skip delete 10" \

> +		    "skip delete 11" \

> +		    "skip delete 12" \

> +		    "skip delete 2" \

> +		    "skip delete 3" \

> +		    "skip delete 4" \

> +		    "skip delete 5" \

> +		    "skip delete 6" \

> +		    "skip delete 7" \

> +		    "skip delete 8" \

> +		    "skip delete 9"]

> +    gdb_test "complete skip delete 1" \

> +	[multi_line "skip delete 1" \

> +		    "skip delete 10" \

> +		    "skip delete 11" \

> +		    "skip delete 12"]

> +    gdb_test "complete skip delete 1 " \

> +	[multi_line "skip delete 1 1" \

> +		    "skip delete 1 10" \

> +		    "skip delete 1 11" \

> +		    "skip delete 1 12" \

> +		    "skip delete 1 2" \

> +		    "skip delete 1 3" \

> +		    "skip delete 1 4" \

> +		    "skip delete 1 5" \

> +		    "skip delete 1 6" \

> +		    "skip delete 1 7" \

> +		    "skip delete 1 8" \

> +		    "skip delete 1 9"]

> +}

Please use the lib/completion-support.exp routines
for testing this.  Those exercise both the complete command
and actual TAB completion.  Above you want to use 
test_gdb_complete_multiple.  

You should also add:

- a test that exercises unique completions like "skip delete 12",
  with test_gdb_complete_unique.

- a test that exercises no completions at all, like "skip delete 2",
  with test_gdb_complete_none.

- some test like "skip delete a1" to make sure we don't
  mistakenly complete that a1 into a1/a10/a11/a12.

As for a range completer, doesn't e.g., "skip delete 1-2<tab>" already
try to complete the "2", and "skip delete 1-" present all the numbers?
I'd think so, given that '-' is part of the default word break chars
set (default_word_break_characters).  You should add tests for that too,
IMO, since people will very naturally use it.  Even if not super smart
(e.g. 2-[TAB] ideally wouldn't present "1"), it's still useful as is.

Thanks,
Pedro Alves
Simon Marchi Nov. 11, 2018, 4:52 p.m. | #2
On 2018-11-10 14:07, Pedro Alves wrote:
> On 11/10/2018 05:39 PM, Simon Marchi wrote:

>> Add completer to various commands that accept skip numbers:

>> 

>>   - skip enable

>>   - skip disable

>>   - skip delete

>>   - info skip

>> 

>> These commands also accept ranges, but I am not too sure of how to do

>> that properly, so I went for the simpler goal of complete just 

>> numbers.

>> 

>> A future idea would be to make a re-usable and well-tested completer 

>> for

>> numbers and ranges.  I think it could at least be re-used for 

>> breakpoint

>> numbers (for example with the "enable breakpoints" command).

> 

> And threads.


Right.


> Please use the lib/completion-support.exp routines

> for testing this.  Those exercise both the complete command

> and actual TAB completion.  Above you want to use

> test_gdb_complete_multiple.

> 

> You should also add:

> 

> - a test that exercises unique completions like "skip delete 12",

>   with test_gdb_complete_unique.

> 

> - a test that exercises no completions at all, like "skip delete 2",

>   with test_gdb_complete_none.

> 

> - some test like "skip delete a1" to make sure we don't

>   mistakenly complete that a1 into a1/a10/a11/a12.

> 

> As for a range completer, doesn't e.g., "skip delete 1-2<tab>" already

> try to complete the "2", and "skip delete 1-" present all the numbers?

> I'd think so, given that '-' is part of the default word break chars

> set (default_word_break_characters).  You should add tests for that 

> too,

> IMO, since people will very naturally use it.  Even if not super smart

> (e.g. 2-[TAB] ideally wouldn't present "1"), it's still useful as is.


Thanks for all the feedback, I hope I have addressed everything in v2.

Simon

Patch

diff --git a/gdb/skip.c b/gdb/skip.c
index 13db0f6b43c6..f2a1df79e8bf 100644
--- a/gdb/skip.c
+++ b/gdb/skip.c
@@ -641,6 +641,23 @@  function_name_is_marked_for_skip (const char *function_name,
   return false;
 }
 
+/* Completer for skip numbers.  */
+
+static void
+complete_skip_number (cmd_list_element *cmd,
+		      completion_tracker &completer,
+		      const char *text, const char *word)
+{
+  size_t word_len = strlen (word);
+
+  for (const skiplist_entry &entry : skiplist_entries)
+    {
+      gdb::unique_xmalloc_ptr<char> name (xstrprintf ("%d", entry.number ()));
+      if (strncmp (word, name.get (), word_len) == 0)
+	completer.add_completion (std::move (name));
+    }
+}
+
 void
 _initialize_step_skip (void)
 {
@@ -676,28 +693,31 @@  If no function name is given, skip the current function."),
 	       &skiplist);
   set_cmd_completer (c, location_completer);
 
-  add_cmd ("enable", class_breakpoint, skip_enable_command, _("\
+  c = add_cmd ("enable", class_breakpoint, skip_enable_command, _("\
 Enable skip entries.  You can specify numbers (e.g. \"skip enable 1 3\"), \
 ranges (e.g. \"skip enable 4-8\"), or both (e.g. \"skip enable 1 3 4-8\").\n\n\
 If you don't specify any numbers or ranges, we'll enable all skip entries.\n\n\
 Usage: skip enable [NUMBER | RANGE]..."),
-	   &skiplist);
+	       &skiplist);
+  set_cmd_completer (c, complete_skip_number);
 
-  add_cmd ("disable", class_breakpoint, skip_disable_command, _("\
+  c = add_cmd ("disable", class_breakpoint, skip_disable_command, _("\
 Disable skip entries.  You can specify numbers (e.g. \"skip disable 1 3\"), \
 ranges (e.g. \"skip disable 4-8\"), or both (e.g. \"skip disable 1 3 4-8\").\n\n\
 If you don't specify any numbers or ranges, we'll disable all skip entries.\n\n\
 Usage: skip disable [NUMBER | RANGE]..."),
-	   &skiplist);
+	       &skiplist);
+  set_cmd_completer (c, complete_skip_number);
 
-  add_cmd ("delete", class_breakpoint, skip_delete_command, _("\
+  c = add_cmd ("delete", class_breakpoint, skip_delete_command, _("\
 Delete skip entries.  You can specify numbers (e.g. \"skip delete 1 3\"), \
 ranges (e.g. \"skip delete 4-8\"), or both (e.g. \"skip delete 1 3 4-8\").\n\n\
 If you don't specify any numbers or ranges, we'll delete all skip entries.\n\n\
 Usage: skip delete [NUMBER | RANGES]..."),
-           &skiplist);
+	       &skiplist);
+  set_cmd_completer (c, complete_skip_number);
 
-  add_info ("skip", info_skip_command, _("\
+  c = add_info ("skip", info_skip_command, _("\
 Display the status of skips.  You can specify numbers (e.g. \"skip info 1 3\"), \
 ranges (e.g. \"skip info 4-8\"), or both (e.g. \"skip info 1 3 4-8\").\n\n\
 If you don't specify any numbers or ranges, we'll show all skips.\n\n\
@@ -705,6 +725,7 @@  Usage: skip info [NUMBER | RANGES]...\n\
 The \"Type\" column indicates one of:\n\
 \tfile        - ignored file\n\
 \tfunction    - ignored function"));
+  set_cmd_completer (c, complete_skip_number);
 
   add_setshow_boolean_cmd ("skip", class_maintenance,
 			   &debug_skip, _("\
diff --git a/gdb/testsuite/gdb.base/skip.exp b/gdb/testsuite/gdb.base/skip.exp
index 223c93d0d9b6..ee328bf35c9b 100644
--- a/gdb/testsuite/gdb.base/skip.exp
+++ b/gdb/testsuite/gdb.base/skip.exp
@@ -16,6 +16,8 @@ 
 # This file was written by Justin Lebar. (justin.lebar@gmail.com)
 # And further hacked on by Doug Evans. (dje@google.com)
 
+standard_testfile
+
 if { [prepare_for_testing "failed to prepare" "skip" \
                           {skip.c skip1.c } \
                           {debug nowarnings}] } {
@@ -297,3 +299,49 @@  with_test_prefix "step using -fi + -fu" {
     gdb_test "step" ".*" "step 4"; # Skip over test_skip()
     gdb_test "step" "test_skip_file_and_function \\(\\) at.*" "step 5"; # Return from skip1_test_skip_file_and_function()
 }
+
+with_test_prefix "skip delete completion" {
+    global binfile
+    clean_restart "${binfile}"
+    if ![runto_main] {
+	fail "can't run to main"
+	return
+    }
+
+    # Create a bunch of skips, don't care what they are.
+    for {set i 0} {$i < 12} {incr i} {
+	gdb_test "skip" ".*" "add skip $i"
+    }
+
+    gdb_test "complete skip delete " \
+	[multi_line "skip delete 1" \
+		    "skip delete 10" \
+		    "skip delete 11" \
+		    "skip delete 12" \
+		    "skip delete 2" \
+		    "skip delete 3" \
+		    "skip delete 4" \
+		    "skip delete 5" \
+		    "skip delete 6" \
+		    "skip delete 7" \
+		    "skip delete 8" \
+		    "skip delete 9"]
+    gdb_test "complete skip delete 1" \
+	[multi_line "skip delete 1" \
+		    "skip delete 10" \
+		    "skip delete 11" \
+		    "skip delete 12"]
+    gdb_test "complete skip delete 1 " \
+	[multi_line "skip delete 1 1" \
+		    "skip delete 1 10" \
+		    "skip delete 1 11" \
+		    "skip delete 1 12" \
+		    "skip delete 1 2" \
+		    "skip delete 1 3" \
+		    "skip delete 1 4" \
+		    "skip delete 1 5" \
+		    "skip delete 1 6" \
+		    "skip delete 1 7" \
+		    "skip delete 1 8" \
+		    "skip delete 1 9"]
+}