Patchwork thread_fsm: Make data fields private (Re: [PATCH] C++-ify struct thread_fsm)

login
register
mail settings
Submitter Pedro Alves
Date Feb. 7, 2019, 3:55 p.m.
Message ID <d8e30902-10c7-85f7-010c-6eb8d5d137dc@redhat.com>
Download mbox | patch
Permalink /patch/31348/
State New
Headers show

Comments

Pedro Alves - Feb. 7, 2019, 3:55 p.m.
On 01/02/2019 11:28 PM, Tom Tromey wrote:
> This C++-ifies struct thread_fsm, replacing the "ops" structure with
> virtual methods, and changing all the implementations to derive from
> thread_fsm.

Thanks for doing this.

> -extern void thread_fsm_set_finished (struct thread_fsm *fsm);
> +  /* The interpreter that issued the execution command that caused
> +     this thread to resume.  If the top level interpreter is MI/async,
> +     and the execution command was a CLI command (next/step/etc.),
> +     we'll want to print stop event output to the MI console channel
> +     (the stepped-to line, etc.), as if the user entered the execution
> +     command on a real GDB console.  */
> +  struct interp *command_interp = nullptr;
>  
> -/* Returns true if the FSM completed successfully.  */
> -extern int thread_fsm_finished_p (struct thread_fsm *fsm);
> +protected:
>  
> -/* Calls the FSM's reply_reason method.  */
> -extern enum async_reply_reason
> -  thread_fsm_async_reply_reason (struct thread_fsm *fsm);
> +  /* Whether the FSM is done successfully.  */
> +  bool finished = false;
>  

This "protected" data field caught my eye.  Doesn't seem to
be any reason for that.  While at it, we can make
command_interp private as well, by adding a getter.

From 3c5b0346b49e6eaedcf467d81b46306e1f4d2c8b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 7 Feb 2019 13:49:04 +0000
Subject: [PATCH] thread_fsm: Make data fields private

Make thread_fsm::finish and command_interp data fields private, and
add a getter for the latter.

gdb/ChangeLog:
2019-02-07  Pedro Alves  <palves@redhat.com>

	* cli/cli-interp.c (should_print_stop_to_console): Adjust.
	* thread-fsm.h (thread_fsm) <thread_fsm>: Adjust and add comment.
	<set_finished, finished>: Adjust.
	<command_interp>: Rename to m_command_interp and make private.
	<command_interp>: Now a method.
	<finished>: Rename to m_finished and make private.
---
 gdb/cli/cli-interp.c |  2 +-
 gdb/thread-fsm.h     | 37 ++++++++++++++++++++++++-------------
 2 files changed, 25 insertions(+), 14 deletions(-)
Tom Tromey - Feb. 11, 2019, 7:40 p.m.
>>>>> "Pedro" == Pedro Alves <palves@redhat.com> writes:

Pedro> This "protected" data field caught my eye.  Doesn't seem to
Pedro> be any reason for that.  While at it, we can make
Pedro> command_interp private as well, by adding a getter.

Thanks, this looks good to me.

Tom

Patch

diff --git a/gdb/cli/cli-interp.c b/gdb/cli/cli-interp.c
index 088f4f1f89..ddbcbd7883 100644
--- a/gdb/cli/cli-interp.c
+++ b/gdb/cli/cli-interp.c
@@ -112,7 +112,7 @@  should_print_stop_to_console (struct interp *console_interp,
   if ((bpstat_what (tp->control.stop_bpstat).main_action
        == BPSTAT_WHAT_STOP_NOISY)
       || tp->thread_fsm == NULL
-      || tp->thread_fsm->command_interp == console_interp
+      || tp->thread_fsm->command_interp () == console_interp
       || !tp->thread_fsm->finished_p ())
     return 1;
   return 0;
diff --git a/gdb/thread-fsm.h b/gdb/thread-fsm.h
index 57837bfdfb..82ad9c2410 100644
--- a/gdb/thread-fsm.h
+++ b/gdb/thread-fsm.h
@@ -30,8 +30,10 @@  struct thread_fsm_ops;
 
 struct thread_fsm
 {
+  /* CMD_INTERP is the interpreter that issued the execution command
+     that caused this thread to resume.  */
   explicit thread_fsm (struct interp *cmd_interp)
-    : command_interp (cmd_interp)
+    : m_command_interp (cmd_interp)
   {
   }
 
@@ -81,33 +83,42 @@  struct thread_fsm
 
   void set_finished ()
   {
-    finished = true;
+    m_finished = true;
   }
 
   bool finished_p () const
   {
-    return finished;
+    return m_finished;
   }
 
-  /* The interpreter that issued the execution command that caused
-     this thread to resume.  If the top level interpreter is MI/async,
-     and the execution command was a CLI command (next/step/etc.),
-     we'll want to print stop event output to the MI console channel
-     (the stepped-to line, etc.), as if the user entered the execution
-     command on a real GDB console.  */
-  struct interp *command_interp = nullptr;
+  /* Return the interpreter that issued the execution command that
+     caused this thread to resume.  */
+  struct interp *command_interp ()
+  {
+    return m_command_interp;
+  }
 
 protected:
 
-  /* Whether the FSM is done successfully.  */
-  bool finished = false;
-
   /* The async_reply_reason that is broadcast to MI clients if this
      FSM finishes successfully.  */
   virtual enum async_reply_reason do_async_reply_reason ()
   {
     gdb_assert_not_reached (_("should not call async_reply_reason here"));
   }
+
+private:
+
+  /* Whether the FSM is done successfully.  */
+  bool m_finished = false;
+
+  /* The interpreter that issued the execution command that caused
+     this thread to resume.  If the top level interpreter is MI/async,
+     and the execution command was a CLI command (next/step/etc.),
+     we'll want to print stop event output to the MI console channel
+     (the stepped-to line, etc.), as if the user entered the execution
+     command on a real GDB console.  */
+  struct interp *m_command_interp = nullptr;
 };
 
 #endif /* THREAD_FSM_H */