This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v2] Class-ify ptid_t
- From: Pedro Alves <palves at redhat dot com>
- To: Simon Marchi <simon dot marchi at ericsson dot com>, gdb-patches at sourceware dot org
- Date: Thu, 6 Apr 2017 23:23:18 +0100
- Subject: Re: [PATCH v2] Class-ify ptid_t
- Authentication-results: sourceware.org; auth=none
- Authentication-results: ext-mx04.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
- Authentication-results: ext-mx04.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=palves at redhat dot com
- Dkim-filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 5D8AC8049B
- Dmarc-filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5D8AC8049B
- References: <20170406190328.21103-1-simon.marchi@ericsson.com>
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, "");
> +
> + /* 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"
> + a process (ptid_is_pid returns true), in which case, all lwps and
"ptid.is_pid ()" ?
> + 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.
> +#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. */
> +
> +static_assert (std::is_pod<ptid_t>::value, "ptid_t is POD");
> +
Otherwise looks good to me. Please push.
Thanks,
Pedro Alves