Re: [PATCH v2] Class-ify ptid_t

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

On 2017-04-06 18:23, Pedro Alves wrote:
> 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, "");

Definitely, good catch.

> > +
> > +  /* 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"

Thanks.

> > +     a process (ptid_is_pid returns true), in which case, all lwps 
> > and
>
> "ptid.is_pid ()" ?

Thanks.

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

Indeed, it's a mistake.  I knew something looked odd with these, but 
couldn't put the finger on why.

> > +#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.  */

Ah, makes sense.  I was only thinking about the instances where ptid_t 
is embedded in structures allocated statically.  In those cases, 
compilation would fail anyway, which is why I didn't really see the 
point of that test.  But of course, it's important for malloc'ed 
structures as well, for which we get now error/warning.

> > +
> > +static_assert (std::is_pod<ptid_t>::value, "ptid_t is POD");
> > +
>
> Otherwise looks good to me.  Please push.

I'll do that later tonight, thanks.

Simon