This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/2] Class-ify ptid_t
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