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

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2017-04-05 11:47, Pedro Alves wrote:
> Hi Simon,
> 
> Hmm, "unit tests or it didn't happen" ? :-)

Right, I don't have the unit test in GDB mindset yet.  But of course, 
it's a good idea, I'll do it.

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

Would you prefer having a ptid_t::from_pid method instead?  It would be 
the counter part of ptid_t::is_pid.  Or do you prefer if we keep the 
current function?

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

Ok.

>> 
>>  /* 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*  :-)

Ok, will look into it.

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

Thanks, fixed

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

Right, it would make sense.

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

Ok.  But anyway C++11-style initialization is probably better anyway.  
Is the following ok?

   struct thread_resume default_action { null_ptid };

Thanks,

Simon