Re: [PATCH, cleanup] Standardize access to ptid

Pedro Alves
2013-09-29T18:45:17Z

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