[PATCH v2] Class-ify ptid_t
Simon Marchi
simon.marchi@polymtl.ca
Fri Apr 7 00:04:00 GMT 2017
On 2017-04-06 18:23, Pedro Alves wrote:
> On 04/06/2017 08:03 PM, Simon Marchi wrote:
>
>> -struct ptid
>> +class ptid_t
>> {
>> +public:
>> + /* Must have a trivial defaulted default constructor so that the
>> + type remains POD. */
>> + ptid_t () noexcept = default;
>> +
>> + /* Make a ptid given the necessary PID, LWP, and TID components.
>> +
>> + A ptid with only a PID (LWP and TID equal to zero) is usually
>> used to
>> + represent a whole process, including all its lwps/threads. */
>> +
>> + constexpr ptid_t (int pid, long lwp = 0, long tid = 0)
>> + : m_pid (pid), m_lwp (lwp), m_tid (tid)
>> + {}
>
> Hmm, I just realized that due to the default arguments, this results
> in an implicit ctor from int, which doesn't sound like a good
> idea to me. I.e., this bug would compile:
>
> void foo (ptid_t ptid);
>
> void bar (int lwpid)
> {
> foo (lwpid); // automatically constructs a (pid,0,0) ptid.
> }
>
> So I think we should make that ctor explicit, and add another assertion
> to the unit tests:
>
> static_assert (!std::is_convertible<int, ptid_t>::value, "");
Definitely, good catch.
>> +
>> + /* Returns true if the ptid matches FILTER. FILTER can be the wild
>> + card MINUS_ONE_PTID (all ptid match it); can be a ptid
>> representing
>
> "all ptids"
Thanks.
>> + a process (ptid_is_pid returns true), in which case, all lwps
>> and
>
> "ptid.is_pid ()" ?
Thanks.
>> + threads of that given process match, lwps and threads of other
>> + processes do not; or, it can represent a specific thread, in
>> which
>> + case, only that thread will match true. The ptid must represent
>> a
>> + specific LWP or THREAD, it can never be a wild card. */
>> +
>> + constexpr bool matches (const ptid_t &filter) const
>> + {
>> + return (/* If filter represents any ptid, it's always a match.
>> */
>> + filter == make_minus_one ()
>> + /* If filter is only a pid, any ptid with that pid
>> + matches. */
>> + || (filter.is_pid () && m_pid == filter.pid ())
>> +
>> + /* Otherwise, this ptid only matches if it's exactly equal
>> + to filter. */
>> + || *this == filter);
>> + }
>> +
>> + /* Make a null ptid. */
>> +
>> + static constexpr ptid_t
>> + make_null ()
>> + { return {0, 0, 0}; }
>> +
>> + /* Make a minus one ptid. */
>> +
>> + static constexpr ptid_t
>> + make_minus_one ()
>> + { return {-1, 0, 0}; }
>
> I find it a bit odd to break the line after the return type in
> these two, when we don't break it in non-static members.
Indeed, it's a mistake. I knew something looked odd with these, but
couldn't put the finger on why.
>> +#include "defs.h"
>> +#include "common/ptid.h"
>> +#include <type_traits>
>> +
>> +namespace selftests {
>> +namespace ptid {
>> +
>> +/* Check that the ptid_t class is POD.
>> +
>> + This isn't a strict requirement. If we have a good reason to
>> change it to
>> + a non-POD type, we can remove this check. */
>
> Hmm, I think this comment too lax. There _is_ a reason this type
> must remain POD for the time being. So I think that's what we
> should say here:
>
> /* Check that the ptid_t class is POD.
>
> This is a requirement for a long as we have ptids embedded in
> structures allocated with malloc. */
Ah, makes sense. I was only thinking about the instances where ptid_t
is embedded in structures allocated statically. In those cases,
compilation would fail anyway, which is why I didn't really see the
point of that test. But of course, it's important for malloc'ed
structures as well, for which we get now error/warning.
>> +
>> +static_assert (std::is_pod<ptid_t>::value, "ptid_t is POD");
>> +
>
> Otherwise looks good to me. Please push.
I'll do that later tonight, thanks.
Simon
More information about the Gdb-patches
mailing list