[PATCH 1/2] ptid_{lwp,tid}_p: Remove unnecessary checks

[PATCH 1/2] ptid_{lwp,tid}_p: Remove unnecessary checks

[Simon Marchi]

The calls to ptid_equal in ptid_lwp_p and ptid_tid_p that compare the
argument to minus_one_ptid and null_ptid are not necessary.  The calls
in question are:

   if (ptid_equal (minus_one_ptid, ptid)
       || ptid_equal (null_ptid, ptid))
     return 0;

minus_one_ptid is { .pid = -1, .lwp = 0, .tid = 0 }
null_ptid is { .pid = 0, .lwp = 0, .tid = 0 }

If the ptid argument is either of them, the statements

  return (ptid_get_lwp (ptid) != 0);

and

  return (ptid_get_tid (ptid) != 0);

will yield the same result (0/false).

gdb/ChangeLog:

	* common/ptid.c (ptid_lwp_p, ptid_tid_p): Remove comparison with
	minus_one_ptid and null_ptid.
---
 gdb/common/ptid.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/gdb/common/ptid.c b/gdb/common/ptid.c
index 05c0db17dd..b56971b9af 100644
--- a/gdb/common/ptid.c
+++ b/gdb/common/ptid.c
@@ -97,10 +97,6 @@ ptid_is_pid (ptid_t ptid)
 int
 ptid_lwp_p (ptid_t ptid)
 {
-  if (ptid_equal (minus_one_ptid, ptid)
-      || ptid_equal (null_ptid, ptid))
-    return 0;
-
   return (ptid_get_lwp (ptid) != 0);
 }
 
@@ -109,10 +105,6 @@ ptid_lwp_p (ptid_t ptid)
 int
 ptid_tid_p (ptid_t ptid)
 {
-  if (ptid_equal (minus_one_ptid, ptid)
-      || ptid_equal (null_ptid, ptid))
-    return 0;
-
   return (ptid_get_tid (ptid) != 0);
 }
 
-- 
2.11.0

[PATCH 2/2] Class-ify ptid_t

[Simon Marchi]

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

gdb/ChangeLog:

	* common/ptid.h (struct ptid): Change to...
	(class ptid_t): ... this.
	<build>: New static method.
	<is_pid, is_null, is_any, pid, lwp_p, lwp, tid_p, tid,
	operator==, matches>: New methods.
	<pid>: Rename to...
	<m_pid>: ...this.
	<lwp>: Rename to...
	<m_lwp>: ...this.
	<tid>: Rename to...
	<m_tid>: ...this.
	(ptid_get_pid, ptid_get_lwp, ptid_get_tid, ptid_equal,
	ptid_is_pid, ptid_lwp_p, ptid_tid_p, ptid_match): Take arguments
	as references.
	* common/ptid.c (null_ptid, minus_one_ptid): Initialize with
	ptid_t::build.
	(ptid_build, pid_to_ptid, ptid_get_pid, ptid_get_tid,
	ptid_equal, ptid_is_pid, ptid_lwp_p, ptid_tid_p, ptid_match):
	Take arguments as references, implement using ptid_t methods.

gdb/gdbserver/ChangeLog:

	* server.c (handle_v_cont): Initialize thread_resume::thread
	with null_ptid.
---
 gdb/common/ptid.c      |  62 +++++++++++-----------------
 gdb/common/ptid.h      | 109 +++++++++++++++++++++++++++++++++++++++----------
 gdb/gdbserver/server.c |   4 +-
 3 files changed, 114 insertions(+), 61 deletions(-)

diff --git a/gdb/common/ptid.c b/gdb/common/ptid.c
index b56971b9af..ca51a4ec7d 100644
--- a/gdb/common/ptid.c
+++ b/gdb/common/ptid.c
@@ -22,20 +22,16 @@
 
 /* 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);
+
 
 /* See ptid.h.  */
 
 ptid_t
 ptid_build (int pid, long lwp, long tid)
 {
-  ptid_t ptid;
-
-  ptid.pid = pid;
-  ptid.lwp = lwp;
-  ptid.tid = tid;
-  return ptid;
+  return ptid_t::build (pid, lwp, tid);
 }
 
 /* See ptid.h.  */
@@ -43,81 +39,69 @@ ptid_build (int pid, long lwp, long tid)
 ptid_t
 pid_to_ptid (int pid)
 {
-  return ptid_build (pid, 0, 0);
+  return ptid_t::build (pid);
 }
 
 /* See ptid.h.  */
 
 int
-ptid_get_pid (ptid_t ptid)
+ptid_get_pid (const ptid_t &ptid)
 {
-  return ptid.pid;
+  return ptid.pid ();
 }
 
 /* See ptid.h.  */
 
 long
-ptid_get_lwp (ptid_t ptid)
+ptid_get_lwp (const ptid_t &ptid)
 {
-  return ptid.lwp;
+  return ptid.lwp ();
 }
 
 /* See ptid.h.  */
 
 long
-ptid_get_tid (ptid_t ptid)
+ptid_get_tid (const ptid_t &ptid)
 {
-  return ptid.tid;
+  return ptid.tid ();
 }
 
 /* See ptid.h.  */
 
 int
-ptid_equal (ptid_t ptid1, ptid_t ptid2)
+ptid_equal (const ptid_t &ptid1, const ptid_t &ptid2)
 {
-  return (ptid1.pid == ptid2.pid
-	  && ptid1.lwp == ptid2.lwp
-	  && ptid1.tid == ptid2.tid);
+  return ptid1 == ptid2;
 }
 
 /* See ptid.h.  */
 
 int
-ptid_is_pid (ptid_t ptid)
+ptid_is_pid (const ptid_t &ptid)
 {
-  if (ptid_equal (minus_one_ptid, ptid)
-      || ptid_equal (null_ptid, ptid))
-    return 0;
-
-  return (ptid_get_lwp (ptid) == 0 && ptid_get_tid (ptid) == 0);
+  return ptid.is_pid ();
 }
 
 /* See ptid.h.  */
 
 int
-ptid_lwp_p (ptid_t ptid)
+ptid_lwp_p (const ptid_t &ptid)
 {
-  return (ptid_get_lwp (ptid) != 0);
+  return ptid.lwp_p ();
 }
 
 /* See ptid.h.  */
 
 int
-ptid_tid_p (ptid_t ptid)
+ptid_tid_p (const ptid_t &ptid)
 {
-  return (ptid_get_tid (ptid) != 0);
+  return ptid.tid_p ();
 }
 
+/* See ptid.h.  */
+
 int
-ptid_match (ptid_t ptid, ptid_t filter)
+ptid_match (const ptid_t &ptid, const ptid_t &filter)
 {
-  if (ptid_equal (filter, minus_one_ptid))
-    return 1;
-  if (ptid_is_pid (filter)
-      && ptid_get_pid (ptid) == ptid_get_pid (filter))
-    return 1;
-  else if (ptid_equal (ptid, filter))
-    return 1;
-
-  return 0;
+  return ptid.matches (filter);
 }
diff --git a/gdb/common/ptid.h b/gdb/common/ptid.h
index 337bfb0899..c8649ae9a8 100644
--- a/gdb/common/ptid.h
+++ b/gdb/common/ptid.h
@@ -20,6 +20,15 @@
 #ifndef PTID_H
 #define PTID_H
 
+class ptid_t;
+
+/* The null or zero ptid, often used to indicate no process.  */
+extern ptid_t null_ptid;
+
+/* The (-1,0,0) ptid, often used to indicate either an error condition
+   or a "don't care" condition, i.e, "run all threads."  */
+extern ptid_t minus_one_ptid;
+
 /* The ptid struct is a collection of the various "ids" necessary for
    identifying the inferior process/thread being debugged.  This
    consists of the process id (pid), lightweight process id (lwp) and
@@ -32,27 +41,85 @@
    thread_stratum target that might want to sit on top.
 */
 
-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())
+      return false;
+
+    return m_lwp == 0 && m_tid == 0;
+  }
+
+  bool is_null () const
+  {
+    return *this == null_ptid;
+  }
+
+  bool is_any () const
+  {
+    return *this == minus_one_ptid;
+  }
+
+  int pid () const
+  { return m_pid; }
+
+  bool lwp_p () const
+  { return m_lwp != 0; }
+
+  long lwp () const
+  { return m_lwp; }
+
+  bool tid_p () const
+  { return m_tid != 0; }
+
+  long tid () const
+  { return m_tid; }
+
+  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
+  {
+    /* 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;
+  }
+
+private:
   /* Process id.  */
-  int pid;
+  int m_pid;
 
   /* Lightweight process id.  */
-  long lwp;
+  long m_lwp;
 
   /* Thread id.  */
-  long tid;
+  long m_tid;
 };
 
-typedef struct ptid ptid_t;
-
-/* The null or zero ptid, often used to indicate no process. */
-extern ptid_t null_ptid;
-
-/* The (-1,0,0) ptid, often used to indicate either an error condition
-   or a "don't care" condition, i.e, "run all threads."  */
-extern ptid_t minus_one_ptid;
-
 /* Make a ptid given the necessary PID, LWP, and TID components.  */
 ptid_t ptid_build (int pid, long lwp, long tid);
 
@@ -61,27 +128,27 @@ ptid_t ptid_build (int pid, long lwp, long tid);
 ptid_t pid_to_ptid (int pid);
 
 /* Fetch the pid (process id) component from a ptid.  */
-int ptid_get_pid (ptid_t ptid);
+int ptid_get_pid (const ptid_t &ptid);
 
 /* Fetch the lwp (lightweight process) component from a ptid.  */
-long ptid_get_lwp (ptid_t ptid);
+long ptid_get_lwp (const ptid_t &ptid);
 
 /* Fetch the tid (thread id) component from a ptid.  */
-long ptid_get_tid (ptid_t ptid);
+long ptid_get_tid (const ptid_t &ptid);
 
 /* Compare two ptids to see if they are equal.  */
-int ptid_equal (ptid_t ptid1, ptid_t ptid2);
+int ptid_equal (const ptid_t &ptid1, const ptid_t &ptid2);
 
 /* Returns true if PTID represents a whole process, including all its
    lwps/threads.  Such ptids have the form of (pid,0,0), with pid !=
    -1.  */
-int ptid_is_pid (ptid_t ptid);
+int ptid_is_pid (const ptid_t &ptid);
 
 /* Return true if PTID's lwp member is non-zero.  */
-int ptid_lwp_p (ptid_t ptid);
+int ptid_lwp_p (const ptid_t &ptid);
 
 /* Return true if PTID's tid member is non-zero.  */
-int ptid_tid_p (ptid_t ptid);
+int ptid_tid_p (const ptid_t &ptid);
 
 /* Returns true if PTID matches filter FILTER.  FILTER can be the wild
    card MINUS_ONE_PTID (all ptid match it); can be a ptid representing
@@ -91,6 +158,6 @@ int ptid_tid_p (ptid_t ptid);
    case, only that thread will match true.  PTID must represent a
    specific LWP or THREAD, it can never be a wild card.  */
 
-extern int ptid_match (ptid_t ptid, ptid_t filter);
+extern int ptid_match (const ptid_t &ptid, const ptid_t &filter);
 
 #endif
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,
+  };
 
   /* Count the number of semicolons in the packet.  There should be one
      for every action.  */
-- 
2.11.0

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

[Pedro Alves]

Hi Simon,

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

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.

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

>  
>  /* 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*  :-)

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

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.

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

Thanks,
Pedro Alves

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

[Simon Marchi]

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

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

[Pedro Alves]

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

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

[Simon Marchi]

On 2017-04-05 17:31, Pedro Alves wrote:
>>> 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.

Ah, so I was just missing the defaulted default constructor.  Adding it 
makes the type trivial, which then makes it POD.

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

It seems to work with a single pair of braces with c++11.  I'll still 
check that it does what we want at runtime, but I'd be surprised if it 
didn't do the right thing.

> 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, "");

Wow, nice.  So all the tests are probably going to be static.

Just to be clear, do you suggest that we make a test that ensures ptid_t 
is a POD, or you wrote that one just to show me it works?  I We don't 
really care if it is, it's just that the current situation (it being 
used in another POD) requires it.

> +
>  /* Make a ptid given the necessary PID, LWP, and TID components.  */
>  ptid_t ptid_build (int pid, long lwp, long tid);

Thanks,

Simon

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

[Pedro Alves]

On 04/06/2017 03:15 AM, Simon Marchi wrote:
> On 2017-04-05 17:31, Pedro Alves wrote:
>>>> 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.
> 
> Ah, so I was just missing the defaulted default constructor.  Adding it
> makes the type trivial, which then makes it POD.

Right, almost.  There's a couple other requirements beyond trivial,
but they're fulfilled as well.  See:

 http://en.cppreference.com/w/cpp/concept/PODType

> 
>>> 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.
> 
> It seems to work with a single pair of braces with c++11.  I'll still
> check that it does what we want at runtime, but I'd be surprised if it
> didn't do the right thing.

Sorry, yes, it's not necessary -- I somehow confused myself into 
thinking that the current double "{{" was because the ptid field was 
inside a structure that itself is inside the thread_resume structure.

struct thread_resume
{
   struct something_else 
   {
      ptid_t thread;


but "something_else" doesn't really exist...  If it existed, then
the double {{ would be necessary in C++11 to initialize the sub field, but
not in C++.   See brace elision here:
http://en.cppreference.com/w/cpp/language/aggregate_initialization

But now that I try, I can't make g++ nor clang++ error in c++11 with
those constructs.  Maybe other compilers would, though.  In any
case, the workaround would be trivial if we needed it -- just
add the equals sign.

>>
>>
>> +static_assert (std::is_pod<ptid_t>::value, "");
>> +
>> +static_assert (ptid_t(1).pid () == 1, "");
> 
> Wow, nice.  So all the tests are probably going to be static.
> 
> Just to be clear, do you suggest that we make a test that ensures ptid_t
> is a POD, or you wrote that one just to show me it works?  I We don't
> really care if it is, it's just that the current situation (it being
> used in another POD) requires it.

Yes, I think we should put that in the unit test with a comment
so that if someone tries to add something that would make it
non-pod, gdb won't even compile.  If/when we get to the point where
we can/want to make it non-pod, we can remove the assertion then.

Thanks,
Pedro Alves

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

[Pedro Alves]

On 04/06/2017 11:49 AM, Pedro Alves wrote:
> Yes, I think we should put that in the unit test with a comment

Or leave it in ptid.c, doesn't have to be in a separate file.
Putting it in the .c file instead of the .h has the advantage
that it doesn't expose type_traits.h to all of gdb, that's all.
(Though I'm not sure whether be able to avoid it as we grow more
C++ utilities.)

> so that if someone tries to add something that would make it
> non-pod, gdb won't even compile.  If/when we get to the point where
> we can/want to make it non-pod, we can remove the assertion then.

Thanks,
Pedro Alves

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

[Simon Marchi]

On 2017-04-06 07:12, Pedro Alves wrote:
> Or leave it in ptid.c, doesn't have to be in a separate file.
> Putting it in the .c file instead of the .h has the advantage
> that it doesn't expose type_traits.h to all of gdb, that's all.
> (Though I'm not sure whether be able to avoid it as we grow more
> C++ utilities.)

My intention was to put the tests in unittests/ptid-selftests.c, any 
objection to that?

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

[Pedro Alves]

On 04/06/2017 03:31 PM, Simon Marchi wrote:
> On 2017-04-06 07:12, Pedro Alves wrote:
>> Or leave it in ptid.c, doesn't have to be in a separate file.
>> Putting it in the .c file instead of the .h has the advantage
>> that it doesn't expose type_traits.h to all of gdb, that's all.
>> (Though I'm not sure whether be able to avoid it as we grow more
>> C++ utilities.)
> 
> My intention was to put the tests in unittests/ptid-selftests.c, any
> objection to that?

No.

Thanks,
Pedro Alves

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

[Simon Marchi]

On 2017-04-05 17:31, Pedro Alves wrote:
> -  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;
>    }

Hmm I think there's a problem with these.  null_ptid and minus_one_ptid 
are not constant expressions, so these methods (and the ones that use 
them) are not really constexpr (you notice when you actually try to use 
them as constant expressions).  Making null_ptid and minus_one_ptid 
constexpr would be useful, I could use them in the static_assert tests 
(e.g. to test "matches").  However, when trying to make them constexpr, 
it creates a dependency loop: the class needs to be defined after the 
null_ptid/minus_one_ptid definitions because it uses them (and you can't 
forward declare a constexpr variable I suppose?), but the 
null_ptid/minus_one_ptid definitions have to be be defined after the 
class, when the type is complete.

An easy workaround would be to not refer to null_ptid/minus_one_ptid in 
the class, but use manually crafted values:

   constexpr bool is_null () const
   {
     return *this == ptid_t (0, 0, 0);
   }

   constexpr bool is_any () const
   {
     return *this == ptid_t (-1, 0, 0);
   }

And to avoid duplication, use defines:

/* Work around the fact that we want to refer to null_ptid and 
minus_one_ptid
    in the definition of class ptid_t, but they have to be defined after. 
  */
#define NULL_PTID ptid (0, 0, 0)
#define MINUS_ONE_PTID ptid (-1, 0, 0)

class ptid_t
{
   ...
   constexpr bool is_null () const
   {
     return *this == NULL_PTID;
   }

   constexpr bool is_any () const
   {
     return *this == MINUS_ONE_PTID;
   }
   ...
}

constexpr ptid_t null_ptid = NULL_PTID;
constexpr ptid_t minus_one_ptid = MINUS_ONE_PTID;

/* We don't want anybody using these macros, they are just temporary.
#undef NULL_PTID
#undef MINUS_ONE_PTID

What do you think?

Now, making null_ptid/minus_one_ptid constexpr brings its share of 
fallouts, such as:

/home/simark/src/binutils-gdb/gdb/linux-nat.c: In function ‘void 
linux_unstop_all_lwps()’:
/home/simark/src/binutils-gdb/gdb/linux-nat.c:2387:37: error: invalid 
conversion from ‘const void*’ to ‘void*’ [-fpermissive]
         resume_stopped_resumed_lwps, &minus_one_ptid);
                                      ^~~~~~~~~~~~~~~
/home/simark/src/binutils-gdb/gdb/linux-nat.c:980:1: note:   
initializing argument 3 of ‘lwp_info* iterate_over_lwps(ptid_t, int 
(*)(lwp_info*, void*), void*)’
  iterate_over_lwps (ptid_t filter,
  ^~~~~~~~~~~~~~~~~

But it looks easy enough to fix by C++-ifying/modernizing 
iterate_over_lwps.

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

[Pedro Alves]

On 04/06/2017 04:09 AM, Simon Marchi wrote:

> constexpr ptid_t null_ptid = NULL_PTID;
> constexpr ptid_t minus_one_ptid = MINUS_ONE_PTID;
> 
> /* We don't want anybody using these macros, they are just temporary.
> #undef NULL_PTID
> #undef MINUS_ONE_PTID
> 
> What do you think?

I think we can avoid the macros.  :-)

Instead, add constexpr functions that make null/any instances, and
call those to initialize the null_ptid/minus_one_ptid globals.

See patchlet below.

> 
> Now, making null_ptid/minus_one_ptid constexpr brings its share of
> fallouts, such as:
> 
> /home/simark/src/binutils-gdb/gdb/linux-nat.c: In function ‘void
> linux_unstop_all_lwps()Â’:
> /home/simark/src/binutils-gdb/gdb/linux-nat.c:2387:37: error: invalid
> conversion from ‘const void*’ to ‘void*’ [-fpermissive]
>         resume_stopped_resumed_lwps, &minus_one_ptid);
>                                      ^~~~~~~~~~~~~~~
> /home/simark/src/binutils-gdb/gdb/linux-nat.c:980:1: note:  
> initializing argument 3 of ‘lwp_info* iterate_over_lwps(ptid_t, int
> (*)(lwp_info*, void*), void*)Â’
>  iterate_over_lwps (ptid_t filter,
>  ^~~~~~~~~~~~~~~~~
> 
> But it looks easy enough to fix by C++-ifying/modernizing
> iterate_over_lwps.

Guess making null_ptid/minus_one_ptid const (even if we avoid
constexpr) would make sense anyway.  Though it's not a huge deal
since the type has no mutating methods in any case.

In any case, I'd be totally fine with modernizing iterate_over_lwps.

From 36aba913c112e0790e7bb832e3010999372c465e Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Thu, 6 Apr 2017 11:50:50 +0100
Subject: [PATCH] make_null

---
 gdb/common/ptid.c |  8 ++++++--
 gdb/common/ptid.h | 10 ++++++++--
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/gdb/common/ptid.c b/gdb/common/ptid.c
index dff0071..d721605 100644
--- a/gdb/common/ptid.c
+++ b/gdb/common/ptid.c
@@ -22,9 +22,13 @@
 
 /* See ptid.h for these.  */
 
-ptid_t null_ptid = ptid_t (0, 0, 0);
-ptid_t minus_one_ptid = ptid_t (-1, 0, 0);
+ptid_t null_ptid = ptid_t::make_null ();
+ptid_t minus_one_ptid = ptid_t::make_any ();
 
+static_assert (ptid_t::make_null ().is_null (), "");
+static_assert (ptid_t::make_any ().is_any (), "");
+static_assert (!ptid_t::make_null ().is_any (), "");
+static_assert (!ptid_t::make_any ().is_null (), "");
 
 /* See ptid.h.  */
 
diff --git a/gdb/common/ptid.h b/gdb/common/ptid.h
index 12d43aa..67e8bad 100644
--- a/gdb/common/ptid.h
+++ b/gdb/common/ptid.h
@@ -65,12 +65,12 @@ public:
 
   constexpr bool is_null () const
   {
-    return *this == null_ptid;
+    return *this == make_null ();
   }
 
   constexpr bool is_any () const
   {
-    return *this == minus_one_ptid;
+    return *this == make_any ();
   }
 
   constexpr int pid () const
@@ -108,6 +108,12 @@ public:
 	    || *this == filter);
   }
 
+  static constexpr ptid_t make_null ()
+  { return {0, 0, 0}; }
+
+  static constexpr ptid_t make_any ()
+  { return {-1, 0, 0}; }
+
 private:
   /* Process id.  */
   int m_pid;
-- 
2.5.5

Re: [PATCH 1/2] ptid_{lwp,tid}_p: Remove unnecessary checks

[Pedro Alves]

On 04/04/2017 07:32 PM, Simon Marchi wrote:
> The calls to ptid_equal in ptid_lwp_p and ptid_tid_p that compare the
> argument to minus_one_ptid and null_ptid are not necessary.  The calls
> in question are:
> 
>    if (ptid_equal (minus_one_ptid, ptid)
>        || ptid_equal (null_ptid, ptid))
>      return 0;
> 
> minus_one_ptid is { .pid = -1, .lwp = 0, .tid = 0 }
> null_ptid is { .pid = 0, .lwp = 0, .tid = 0 }
> 
> If the ptid argument is either of them, the statements
> 
>   return (ptid_get_lwp (ptid) != 0);
> 
> and
> 
>   return (ptid_get_tid (ptid) != 0);
> 
> will yield the same result (0/false).
> 
> gdb/ChangeLog:
> 
> 	* common/ptid.c (ptid_lwp_p, ptid_tid_p): Remove comparison with
> 	minus_one_ptid and null_ptid.

Indeed.  LGTM.

Thanks,
Pedro Alves

Re: [PATCH 1/2] ptid_{lwp,tid}_p: Remove unnecessary checks

[Simon Marchi]

On 2017-04-05 11:15, Pedro Alves wrote:
> On 04/04/2017 07:32 PM, Simon Marchi wrote:
>> The calls to ptid_equal in ptid_lwp_p and ptid_tid_p that compare the
>> argument to minus_one_ptid and null_ptid are not necessary.  The calls
>> in question are:
>> 
>>    if (ptid_equal (minus_one_ptid, ptid)
>>        || ptid_equal (null_ptid, ptid))
>>      return 0;
>> 
>> minus_one_ptid is { .pid = -1, .lwp = 0, .tid = 0 }
>> null_ptid is { .pid = 0, .lwp = 0, .tid = 0 }
>> 
>> If the ptid argument is either of them, the statements
>> 
>>   return (ptid_get_lwp (ptid) != 0);
>> 
>> and
>> 
>>   return (ptid_get_tid (ptid) != 0);
>> 
>> will yield the same result (0/false).
>> 
>> gdb/ChangeLog:
>> 
>> 	* common/ptid.c (ptid_lwp_p, ptid_tid_p): Remove comparison with
>> 	minus_one_ptid and null_ptid.
> 
> Indeed.  LGTM.
> 
> Thanks,
> Pedro Alves

Thanks, this one is pushed.