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

[Pedro Alves <palves@redhat.com>]

On 04/05/2017 08:44 PM, Simon Marchi wrote:
> 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?

Either of those sounds fine.  Or maybe that's not a big deal, and
a ctor that takes a single "pid" would be fine.  See below.

>>> 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.

Yeah, also, the compiler will probably be able to inline these
most of the times.

> 
>>>
>>>  /* 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.

Thanks.  constexpr in C++11 forces you to write a single
return statement (C++14 gets rid of that requirement),
but it looks quite doable.

Also, note that it's not true that this type can't have a
constructor.  It can, as long as the type remains POD.

I've pasted below a quick patch on top of yours to get
you going.  Note the static_asserts.

>> 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 };

ISTR that in C++11 you'll need the double "{{" levels, like:

   thread_resume default_action {{null_ptid}};

and that C++14 removed that requirement (brace elision).
But I haven't confirmed.  Whatever works with -std=c++11.

From 63f3c4ed145a069df68991fabbf025ffaedf8cf6 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 5 Apr 2017 22:21:58 +0100
Subject: [PATCH] POD

---
 gdb/common/ptid.c |  8 +++----
 gdb/common/ptid.h | 68 ++++++++++++++++++++++++++++---------------------------
 2 files changed, 39 insertions(+), 37 deletions(-)

diff --git a/gdb/common/ptid.c b/gdb/common/ptid.c
index ca51a4e..dff0071 100644
--- a/gdb/common/ptid.c
+++ b/gdb/common/ptid.c
@@ -22,8 +22,8 @@
 
 /* See ptid.h for these.  */
 
-ptid_t null_ptid = ptid_t::build (0, 0, 0);
-ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0);
+ptid_t null_ptid = ptid_t (0, 0, 0);
+ptid_t minus_one_ptid = ptid_t (-1, 0, 0);
 
 
 /* See ptid.h.  */
@@ -31,7 +31,7 @@ ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0);
 ptid_t
 ptid_build (int pid, long lwp, long tid)
 {
-  return ptid_t::build (pid, lwp, tid);
+  return ptid_t (pid, lwp, tid);
 }
 
 /* See ptid.h.  */
@@ -39,7 +39,7 @@ ptid_build (int pid, long lwp, long tid)
 ptid_t
 pid_to_ptid (int pid)
 {
-  return ptid_t::build (pid);
+  return ptid_t (pid);
 }
 
 /* See ptid.h.  */
diff --git a/gdb/common/ptid.h b/gdb/common/ptid.h
index c8649ae..1cb57e1 100644
--- a/gdb/common/ptid.h
+++ b/gdb/common/ptid.h
@@ -20,6 +20,8 @@
 #ifndef PTID_H
 #define PTID_H
 
+#include <type_traits>
+
 class ptid_t;
 
 /* The null or zero ptid, often used to indicate no process.  */
@@ -44,69 +46,65 @@ extern ptid_t minus_one_ptid;
 class ptid_t
 {
 public:
-  static ptid_t build (int pid, long lwp = 0, long tid = 0)
-  {
-    ptid_t ptid;
+  /* Must have a trivial defaulted default constructor so that the
+     type remains POD.  */
+  ptid_t () noexcept = default;
 
-    ptid.m_pid = pid;
-    ptid.m_lwp = lwp;
-    ptid.m_tid = tid;
+  constexpr ptid_t (int pid, long lwp = 0, long tid = 0)
+    : m_pid (pid), m_lwp (lwp), m_tid (tid)
+  {}
 
-    return ptid;
-  }
-
-  bool is_pid () const
+  constexpr bool is_pid () const
   {
-    if (is_any () || is_null())
-      return false;
-
-    return m_lwp == 0 && m_tid == 0;
+    return (!is_any ()
+	    && !is_null()
+	    && m_lwp == 0
+	    && m_tid == 0);
   }
 
-  bool is_null () const
+  constexpr bool is_null () const
   {
     return *this == null_ptid;
   }
 
-  bool is_any () const
+  constexpr bool is_any () const
   {
     return *this == minus_one_ptid;
   }
 
-  int pid () const
+  constexpr int pid () const
   { return m_pid; }
 
-  bool lwp_p () const
+  constexpr bool lwp_p () const
   { return m_lwp != 0; }
 
-  long lwp () const
+  constexpr long lwp () const
   { return m_lwp; }
 
-  bool tid_p () const
+  constexpr bool tid_p () const
   { return m_tid != 0; }
 
-  long tid () const
+  constexpr long tid () const
   { return m_tid; }
 
-  bool operator== (const ptid_t &other) const
+  constexpr bool operator== (const ptid_t &other) const
   {
     return (m_pid == other.m_pid
 	    && m_lwp == other.m_lwp
 	    && m_tid == other.m_tid);
   }
 
-  bool matches (const ptid_t &filter) const
+  constexpr bool matches (const ptid_t &filter) const
   {
-    /* If filter represents any ptid, it's always a match.  */
-    if (filter.is_any ())
-      return true;
-
-    /* If filter is only a pid, any ptid with that pid matches.  */
-    if (filter.is_pid () && m_pid == filter.pid ())
-      return true;
-
-    /* Otherwise, this ptid only matches if it's exactly equal to filter.  */
-    return *this == filter;
+    return (/* If filter represents any ptid, it's always a match.  */
+	    filter.is_any ()
+	    /* If filter is only a pid, any ptid with that pid
+	       matches.  */
+	    || (filter.is_pid () && m_pid == filter.pid ())
+
+	    /* Otherwise, this ptid only matches if it's exactly equal
+	       to filter.  */
+	    || *this == filter);
   }
 
 private:
@@ -120,6 +118,10 @@ private:
   long m_tid;
 };
 
+static_assert (std::is_pod<ptid_t>::value, "");
+
+static_assert (ptid_t(1).pid () == 1, "");
+
 /* Make a ptid given the necessary PID, LWP, and TID components.  */
 ptid_t ptid_build (int pid, long lwp, long tid);
 
-- 
2.5.5