Re: [PATCH, cleanup] Standardize access to ptid

[Pedro Alves <palves at redhat dot com>]

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!

-- 
Pedro Alves