Re: [PATCH v2] Class-ify ptid_t

[Simon Marchi <simon.marchi@polymtl.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