Re: [PATCH, cleanup] Standardize access to ptid

[Luis Machado <lgustavo@codesourcery.com>]

On 29-09-2013 15:45, Pedro Alves wrote:
> On 09/29/2013 06:16 PM, Luis Machado wrote:
>
>>>> @@ -33,6 +33,10 @@
>>>>          ptid_get_lwp	- Fetch the lwp component of a ptid.
>>>>          ptid_get_tid	- Fetch the tid component of a ptid.
>>>>          ptid_equal	- Test to see if two ptids are equal.
>>>> +      ptid_is_pid	- Test if a ptid's pid component is non-zero.
>>>
>>> No, that's not right:
>>>
>>> /* Returns true if PTID represents a process.  */
>>>
>>> int
>>> ptid_is_pid (ptid_t ptid)
>>> {
>>>     if (ptid_equal (minus_one_ptid, ptid))
>>>       return 0;
>>>     if (ptid_equal (null_ptid, ptid))
>>>       return 0;
>>>
>>>     return (ptid_get_lwp (ptid) == 0 && ptid_get_tid (ptid) == 0);
>>> }
>>>
>>> So this only returns true iff the ptid looks like (pid,0,0).
>>> (ptid_is_pid on (pid,lwp,0) returns false, for example.)
>>> This is considered a ptid that identifies the whole PID process (the
>>> whole thread group in Linux speak).  Both the core and the targets
>>> use and agree on this.
>>
>> I've changed the description to the following:
>>
>> "Test if a ptid looks like (pid, 0, 0)."
>>
>> Seems to clearly state what is being checked.
>
> Sounds fine, thanks.
>
>>> ...
>
>> All of this makes sense to me, but perhaps we should introduce such a
>> change later on? After the cleanup possibly, since this will require
>> changes in places of the code that deal with various subsystems of GDB.
>
> Yes, I was just doing a brain dump.  I'm not suggesting to actually
> do it now.  And certainly not ever as part of this patch.
>
>>> With that in mind, I think I'd prefer renaming these
>>> new "is" functions as:
>>>
>>>    ptid_is_lwp -> ptid_lwp_p
>>>    ptid_is_tid -> ptid_tid_p
>>>
>>> (or at least ptid_has_lwp, though the _p variant has
>>> precedent in the frame stuff, and it feels to me that frame_ids
>>> and ptids are at about the same conceptual level.)
>>
>> I'm happy with ptid_lwp_p and ptid_tid_p.
>
> Thanks.
>
>>> And I'm also don't really like the "ptid_is_invalid" function that much.
>>> minus_one_ptid or null_ptid aren't really always invalid.  They have
>>> special meanings as either invalid, terminator, or as wildcard depending
>>> on context.  See e.g, how frame_id_p returns true to wildcard frame ids,
>>> and the special outer_frame_id (although that one should die.
>>> But with the above suggestion, I don't think the function
>>> would end up with any use left, so it could just be dropped.  But I suppose
>>> I'll just get used to it if it stays.  ;-)  But, if it stays, please,
>>> please, invert its logic, getting rid of the double
>>> negative ("if (!ptid_is_invalid ()").
>>
>> I thought about ptid_special_p or ptid_is_special to check for both
>
> Yeah, good one.  I would have liked that naming more.
>
>> null_ptid and minus_one_ptid, but this check would only be used (for now
>> at least) in the ptid.c file. Maybe not worth the effort, so i left it out.
>
> Yeah.
>
> I only skimmed most of the new patch, focused mostly on ptid.c/ptid.h, and
> on the places is_lwp / is_thread used to be used, and, I'm fine with this
> version.  Thanks a lot!
>

Checked in. Thanks!

Luis