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
- From: Pedro Alves <palves at redhat dot com>
- To: Simon Marchi <simon dot marchi at ericsson dot com>, gdb-patches at sourceware dot org
- Date: Wed, 5 Apr 2017 16:47:13 +0100
- Subject: Re: [PATCH 2/2] Class-ify ptid_t
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 257413DBD9
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 257413DBD9
- References: <20170404183235.10589-1-simon.marchi@ericsson.com> <20170404183235.10589-2-simon.marchi@ericsson.com>
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