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

[Pedro Alves <palves at redhat dot 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