From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 62228 invoked by alias); 7 Apr 2017 00:04:32 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Received: (qmail 62214 invoked by uid 89); 7 Apr 2017 00:04:32 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=no version=3.3.2 spammy=finger, card, tonight X-HELO: simark.ca Received: from simark.ca (HELO simark.ca) (158.69.221.121) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 07 Apr 2017 00:04:29 +0000 Received: by simark.ca (Postfix, from userid 33) id 5D2091E80F; Thu, 6 Apr 2017 20:04:29 -0400 (EDT) To: Pedro Alves Subject: Re: [PATCH v2] Class-ify ptid_t X-PHP-Originating-Script: 33:rcube.php MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Fri, 07 Apr 2017 00:04:00 -0000 From: Simon Marchi Cc: Simon Marchi , gdb-patches@sourceware.org In-Reply-To: <78ec4cd7-8a6e-d757-cb1f-de7e3bb52aab@redhat.com> References: <20170406190328.21103-1-simon.marchi@ericsson.com> <78ec4cd7-8a6e-d757-cb1f-de7e3bb52aab@redhat.com> Message-ID: <01ede052c2a7f8717e338e3e4de20a41@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.4 X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00151.txt.bz2 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::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 >> + >> +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::value, "ptid_t is POD"); >> + > > Otherwise looks good to me. Please push. I'll do that later tonight, thanks. Simon