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

[Simon Marchi <simon dot marchi at polymtl dot ca>]

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