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!