This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 2/2] Class-ify ptid_t


On 2017-04-05 11:47, Pedro Alves wrote:
Hi Simon,

Hmm, "unit tests or it didn't happen" ? :-)

Right, I don't have the unit test in GDB mindset yet. But of course, it's a good idea, I'll do it.

On 04/04/2017 07:32 PM, Simon Marchi wrote:
I grew a bit tired of using ptid_get_{lwp,pid,tid} and friends, so I
decided to make it a bit easier to use by making it a proper class.

Because ptid_t is used in things that aren't constructed, it is not
possible to have a constructor.  Instead I added a "build" static
method, which maps well to the current ptid_build anyway, and ptid_t is
basically just a plain old data type with read-only methods.  The
difference with before is that the fields are private, so it's not
possible to change a ptid_t field by mistake.

The new methods of ptid_t map to existing functions/practice like this:

  ptid_t::build (pid, lwp, tid) -> ptid_build (pid, lwp, tid)
  ptid_t::build (pid) -> pid_to_ptid (pid)

Not sure these two are an improvement.  pid_to_ptid is the
counterpart of ptid_is_pid, and that is lost with the
overloading of ptid_t::build.

Would you prefer having a ptid_t::from_pid method instead? It would be the counter part of ptid_t::is_pid. Or do you prefer if we keep the current function?

  ptid.is_pid () -> ptid_is_pid (ptid)
  ptid == other -> ptid_equal (ptid, other)
  ptid.is_null () -> ptid_equal (ptid, null_ptid)
  ptid.is_any () -> ptid_equal (ptid, minus_one_ptid)
  ptid.pid () -> ptid_get_pid (ptid)
  ptid.lwp_p () -> ptid_lwp_p (ptid)
  ptid.lwp () -> ptid_get_lwp (ptid)
  ptid.tid_p () -> ptid_tid_p (ptid)
  ptid.tid () -> ptid_get_tid (ptid)
  ptid.matches (filter) -> ptid_match (ptid, filter)

I've replaced the implementation of the existing functions with calls to the new methods. People are encouraged to gradually switch to using the
ptid_t methods instead of the functions (or we can change them all in
one pass eventually).

Also, I'm not sure if it's worth it (because of ptid_t's relatively
small size), but I have made the functions and methods take ptid_t
arguments by const reference instead of by value.

I'd guess that the structure is still sufficiently small that passing
by value would be a benefit (plus, it avoids inefficiency caused
by the compiler having to assume that the references can alias),
but OTOH, this structure is likely to grow with the multi-target
work.  Fine with me to go with what you have.

Ok.


 /* See ptid.h for these.  */

-ptid_t null_ptid = { 0, 0, 0 };
-ptid_t minus_one_ptid = { -1, 0, 0 };
+ptid_t null_ptid = ptid_t::build (0, 0, 0);
+ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0);

It's probably going to be worth it to sprinkle "constexpr"
all over the new API.  Helps with static_asserts in
unit testing too.  *cough*  :-)

Ok, will look into it.

-struct ptid
+class ptid_t
 {
+public:
+  static ptid_t build (int pid, long lwp = 0, long tid = 0)
+  {
+    ptid_t ptid;
+
+    ptid.m_pid = pid;
+    ptid.m_lwp = lwp;
+    ptid.m_tid = tid;
+
+    return ptid;
+  }
+
+  bool is_pid () const
+  {
+    if (is_any () || is_null())

Missing space after "null".

Thanks, fixed

Wonder about migrating/copying the comments API comments to
the methods, if these are the entry points that people should
be looking at going forward.

Right, it would make sense.

+      return false;
+
+    return m_lwp == 0 && m_tid == 0;
+  }
+

diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c
index 4bc7f71b00..1287114cc1 100644
--- a/gdb/gdbserver/server.c
+++ b/gdb/gdbserver/server.c
@@ -2654,7 +2654,9 @@ handle_v_cont (char *own_buf)
   char *p, *q;
   int n = 0, i = 0;
   struct thread_resume *resume_info;
-  struct thread_resume default_action = {{0}};
+  struct thread_resume default_action = {
+    .thread = null_ptid,
+  };

Note that C99 designated initializers are not valid C++11.
Not sure whether any compiler _doesn't_ support them though.

Ok. But anyway C++11-style initialization is probably better anyway. Is the following ok?

  struct thread_resume default_action { null_ptid };

Thanks,

Simon


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]