Consolidate the custom TUI query hook with default query hook

Message ID 1420730248-18521-1-git-send-email-patrick@parcs.ath.cx
State New, archived
Headers

Commit Message

Patrick Palka Jan. 8, 2015, 3:17 p.m. UTC
  Hi Pedro,

This is what I have so far.  It seems to work well.  Thoughts?

-- >8 --

This patch primarily rewrites defaulted_query() to use
gdb_readline_wrapper() to prompt the user for input, like
prompt_for_continue() does.  The motivation for this rewrite is to be
able to reuse the default query hook in TUI, obviating the need for a
custom TUI query hook.

gdb/ChangeLog:

	* utils.c (defaulted_query): Rewrite to use gdb_readline_wrapper
	to prompt for input.
	* tui/tui-hooks.c (tui_query_hook): Remove.
	(tui_install_hooks): Don't set deprecated_query_hook.
---
 gdb/tui/tui-hooks.c | 64 -----------------------------------------------------
 gdb/utils.c         | 56 ++++++++++------------------------------------
 2 files changed, 12 insertions(+), 108 deletions(-)
  

Comments

Pedro Alves Jan. 8, 2015, 3:50 p.m. UTC | #1
On 01/08/2015 03:17 PM, Patrick Palka wrote:
> Hi Pedro,
> 
> This is what I have so far.  It seems to work well.  Thoughts?

Nice!

> --- a/gdb/utils.c
> +++ b/gdb/utils.c
> @@ -1198,12 +1198,11 @@ compile_rx_or_error (regex_t *pattern, const char *rx, const char *message)
>  static int ATTRIBUTE_PRINTF (1, 0)
>  defaulted_query (const char *ctlstr, const char defchar, va_list args)
>  {
> -  int answer;
>    int ans2;
>    int retval;
>    int def_value;
>    char def_answer, not_def_answer;
> -  char *y_string, *n_string, *question;
> +  char *y_string, *n_string, *question, *prompt;
>    /* Used to add duration we waited for user to respond to
>       prompt_for_continue_wait_time.  */
>    struct timeval prompt_started, prompt_ended, prompt_delta;
> @@ -1263,62 +1262,31 @@ defaulted_query (const char *ctlstr, const char defchar, va_list args)
>  
>    /* Format the question outside of the loop, to avoid reusing args.  */
>    question = xstrvprintf (ctlstr, args);
> +  prompt = xstrprintf ("%s%s(%s or %s) %s",

Add _() here, for the "or".

> -      printf_filtered (_("(%s or %s) "), y_string, n_string);

Notice here we had it.

> -
> -      if (annotation_level > 1)
> -	printf_filtered (("\n\032\032query\n"));
> +      char *response, answer = EOF;
>  
> -      wrap_here ("");
>        gdb_flush (gdb_stdout);
> +      response = gdb_readline_wrapper (prompt);
> +      if (response != NULL)
> +	answer = response[0];
> +      xfree (response);
>  
> -      answer = fgetc (stdin);
> -
> -      /* We expect fgetc to block until a character is read.  But
> -         this may not be the case if the terminal was opened with
> -         the NONBLOCK flag.  In that case, if there is nothing to
> -         read on stdin, fgetc returns EOF, but also sets the error
> -         condition flag on stdin and errno to EAGAIN.  With a true
> -         EOF, stdin's error condition flag is not set.
> -
> -         A situation where this behavior was observed is a pseudo
> -         terminal on AIX.  */
> -      while (answer == EOF && ferror (stdin) && errno == EAGAIN)
> -        {
> -          /* Not a real EOF.  Wait a little while and try again until
> -             we read something.  */
> -          clearerr (stdin);
> -          gdb_usleep (10000);
> -          answer = fgetc (stdin);
> -        }
> -
> -      clearerr (stdin);		/* in case of C-d */
>        if (answer == EOF)	/* C-d */
>  	{
>  	  printf_filtered ("EOF [assumed %c]\n", def_answer);
>  	  retval = def_value;
>  	  break;
>  	}

Can you move this a bit above, and write the check in
terms of response, like this?

        if (response == NULL)	/* C-d */
  	{
  	  printf_filtered ("EOF [assumed %c]\n", def_answer);
  	  retval = def_value;
  	  break;
  	}

I think that's a little bit clearer.  What got me to that was that
you made "answer" a char (from int), which may be signed or unsigned,
depending on host, and then the >= 'a' check further below looked
suspicious, though I see that it's unreachable for the EOF case.  I think
that way there'll no be reference to EOF at all in the
code (except in the printf string).

Other than that, if this passes testing, that it's OK with me.

Thanks for doing this.
  

Patch

diff --git a/gdb/tui/tui-hooks.c b/gdb/tui/tui-hooks.c
index 6ba6285..e53f526 100644
--- a/gdb/tui/tui-hooks.c
+++ b/gdb/tui/tui-hooks.c
@@ -63,68 +63,6 @@  tui_new_objfile_hook (struct objfile* objfile)
     tui_display_main ();
 }
 
-static int ATTRIBUTE_PRINTF (1, 0)
-tui_query_hook (const char *msg, va_list argp)
-{
-  int retval;
-  int ans2;
-  int answer;
-  char *question;
-  struct cleanup *old_chain;
-
-  /* Format the question outside of the loop, to avoid reusing
-     ARGP.  */
-  question = xstrvprintf (msg, argp);
-  old_chain = make_cleanup (xfree, question);
-
-  echo ();
-  while (1)
-    {
-      wrap_here ("");		/* Flush any buffered output.  */
-      gdb_flush (gdb_stdout);
-
-      fputs_filtered (question, gdb_stdout);
-      printf_filtered (_("(y or n) "));
-
-      wrap_here ("");
-      gdb_flush (gdb_stdout);
-
-      answer = tui_getc (stdin);
-      clearerr (stdin);		/* in case of C-d */
-      if (answer == EOF)	/* C-d */
-	{
-	  retval = 1;
-	  break;
-	}
-      /* Eat rest of input line, to EOF or newline.  */
-      if (answer != '\n')
-	do
-	  {
-            ans2 = tui_getc (stdin);
-	    clearerr (stdin);
-	  }
-	while (ans2 != EOF && ans2 != '\n' && ans2 != '\r');
-
-      if (answer >= 'a')
-	answer -= 040;
-      if (answer == 'Y')
-	{
-	  retval = 1;
-	  break;
-	}
-      if (answer == 'N')
-	{
-	  retval = 0;
-	  break;
-	}
-      printf_filtered (_("Please answer y or n.\n"));
-    }
-  noecho ();
-
-  do_cleanups (old_chain);
-  return retval;
-}
-
 /* Prevent recursion of deprecated_register_changed_hook().  */
 static int tui_refreshing_registers = 0;
 
@@ -263,8 +201,6 @@  tui_install_hooks (void)
   deprecated_print_frame_info_listing_hook
     = tui_print_frame_info_listing_hook;
 
-  deprecated_query_hook = tui_query_hook;
-
   /* Install the event hooks.  */
   tui_bp_created_observer
     = observer_attach_breakpoint_created (tui_event_create_breakpoint);
diff --git a/gdb/utils.c b/gdb/utils.c
index 084db87..187f91a 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -1198,12 +1198,11 @@  compile_rx_or_error (regex_t *pattern, const char *rx, const char *message)
 static int ATTRIBUTE_PRINTF (1, 0)
 defaulted_query (const char *ctlstr, const char defchar, va_list args)
 {
-  int answer;
   int ans2;
   int retval;
   int def_value;
   char def_answer, not_def_answer;
-  char *y_string, *n_string, *question;
+  char *y_string, *n_string, *question, *prompt;
   /* Used to add duration we waited for user to respond to
      prompt_for_continue_wait_time.  */
   struct timeval prompt_started, prompt_ended, prompt_delta;
@@ -1263,62 +1262,31 @@  defaulted_query (const char *ctlstr, const char defchar, va_list args)
 
   /* Format the question outside of the loop, to avoid reusing args.  */
   question = xstrvprintf (ctlstr, args);
+  prompt = xstrprintf ("%s%s(%s or %s) %s",
+		      annotation_level > 1 ? "\n\032\032pre-query\n" : "",
+		      question, y_string, n_string,
+		      annotation_level > 1 ? "\n\032\032query\n" : "");
+  xfree (question);
 
   /* Used for calculating time spend waiting for user.  */
   gettimeofday (&prompt_started, NULL);
 
   while (1)
     {
-      wrap_here ("");		/* Flush any buffered output.  */
-      gdb_flush (gdb_stdout);
-
-      if (annotation_level > 1)
-	printf_filtered (("\n\032\032pre-query\n"));
-
-      fputs_filtered (question, gdb_stdout);
-      printf_filtered (_("(%s or %s) "), y_string, n_string);
-
-      if (annotation_level > 1)
-	printf_filtered (("\n\032\032query\n"));
+      char *response, answer = EOF;
 
-      wrap_here ("");
       gdb_flush (gdb_stdout);
+      response = gdb_readline_wrapper (prompt);
+      if (response != NULL)
+	answer = response[0];
+      xfree (response);
 
-      answer = fgetc (stdin);
-
-      /* We expect fgetc to block until a character is read.  But
-         this may not be the case if the terminal was opened with
-         the NONBLOCK flag.  In that case, if there is nothing to
-         read on stdin, fgetc returns EOF, but also sets the error
-         condition flag on stdin and errno to EAGAIN.  With a true
-         EOF, stdin's error condition flag is not set.
-
-         A situation where this behavior was observed is a pseudo
-         terminal on AIX.  */
-      while (answer == EOF && ferror (stdin) && errno == EAGAIN)
-        {
-          /* Not a real EOF.  Wait a little while and try again until
-             we read something.  */
-          clearerr (stdin);
-          gdb_usleep (10000);
-          answer = fgetc (stdin);
-        }
-
-      clearerr (stdin);		/* in case of C-d */
       if (answer == EOF)	/* C-d */
 	{
 	  printf_filtered ("EOF [assumed %c]\n", def_answer);
 	  retval = def_value;
 	  break;
 	}
-      /* Eat rest of input line, to EOF or newline.  */
-      if (answer != '\n')
-	do
-	  {
-	    ans2 = fgetc (stdin);
-	    clearerr (stdin);
-	  }
-	while (ans2 != EOF && ans2 != '\n' && ans2 != '\r');
 
       if (answer >= 'a')
 	answer -= 040;
@@ -1350,7 +1318,7 @@  defaulted_query (const char *ctlstr, const char defchar, va_list args)
   timeval_add (&prompt_for_continue_wait_time,
                &prompt_for_continue_wait_time, &prompt_delta);
 
-  xfree (question);
+  xfree (prompt);
   if (annotation_level > 1)
     printf_filtered (("\n\032\032post-query\n"));
   return retval;