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