Re: [PATCH v2] Class-ify ptid_t

[Pedro Alves <palves@redhat.com>]

On 04/06/2017 08:03 PM, Simon Marchi wrote:

> -struct ptid
> +class ptid_t
>  {
> +public:
> +  /* Must have a trivial defaulted default constructor so that the
> +     type remains POD.  */
> +  ptid_t () noexcept = default;
> +
> +  /* Make a ptid given the necessary PID, LWP, and TID components.
> +
> +     A ptid with only a PID (LWP and TID equal to zero) is usually used to
> +     represent a whole process, including all its lwps/threads.  */
> +
> +  constexpr ptid_t (int pid, long lwp = 0, long tid = 0)
> +    : m_pid (pid), m_lwp (lwp), m_tid (tid)
> +  {}

Hmm, I just realized that due to the default arguments, this results
in an implicit ctor from int, which doesn't sound like a good
idea to me.  I.e., this bug would compile:

 void foo (ptid_t ptid);

 void bar (int lwpid)
 {
   foo (lwpid); // automatically constructs a (pid,0,0) ptid.
 }

So I think we should make that ctor explicit, and add another assertion
to the unit tests:

  static_assert (!std::is_convertible<int, ptid_t>::value, "");

> +
> +  /* Returns true if the ptid matches FILTER.  FILTER can be the wild
> +     card MINUS_ONE_PTID (all ptid match it); can be a ptid representing

"all ptids"

> +     a process (ptid_is_pid returns true), in which case, all lwps and

"ptid.is_pid ()" ?

> +     threads of that given process match, lwps and threads of other
> +     processes do not; or, it can represent a specific thread, in which
> +     case, only that thread will match true.  The ptid must represent a
> +     specific LWP or THREAD, it can never be a wild card.  */
> +
> +  constexpr bool matches (const ptid_t &filter) const
> +  {
> +    return (/* If filter represents any ptid, it's always a match.  */
> +	    filter == make_minus_one ()
> +	    /* 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);
> +  }
> +
> +  /* Make a null ptid.  */
> +
> +  static constexpr ptid_t
> +  make_null ()
> +  { return {0, 0, 0}; }
> +
> +  /* Make a minus one ptid.  */
> +
> +  static constexpr ptid_t
> +  make_minus_one ()
> +  { return {-1, 0, 0}; }

I find it a bit odd to break the line after the return type in
these two, when we don't break it in non-static members.

> +#include "defs.h"
> +#include "common/ptid.h"
> +#include <type_traits>
> +
> +namespace selftests {
> +namespace ptid {
> +
> +/* Check that the ptid_t class is POD.
> +
> +   This isn't a strict requirement.  If we have a good reason to change it to
> +   a non-POD type, we can remove this check.  */

Hmm, I think this comment too lax.  There _is_ a reason this type
must remain POD for the time being.  So I think that's what we
should say here:

/* Check that the ptid_t class is POD.

   This is a requirement for a long as we have ptids embedded in
   structures allocated with malloc.  */

> +
> +static_assert (std::is_pod<ptid_t>::value, "ptid_t is POD");
> +

Otherwise looks good to me.  Please push.

Thanks,
Pedro Alves