From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 106562 invoked by alias); 5 Apr 2017 21:31:25 -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 106552 invoked by uid 89); 5 Apr 2017 21:31:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.0 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 spammy=pod X-HELO: mail-wr0-f171.google.com Received: from mail-wr0-f171.google.com (HELO mail-wr0-f171.google.com) (209.85.128.171) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 05 Apr 2017 21:31:22 +0000 Received: by mail-wr0-f171.google.com with SMTP id w11so31558821wrc.3 for ; Wed, 05 Apr 2017 14:31:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=DfbzUgm3cAvrvY4O+xKX3Oef/tKoRtsMaMcpr2sbwGg=; b=GuyqmtgX221lpQ4s0cR76qYXdKvzcjhbtK9/dFhFedJJaCbdxVfapLz+8slDC2R4U1 lTklrA763/cWc3UuW5DDWvlBiMhVXwZqSRElmoBvGPdOrQu5qpUVbSCnax+vgA/cMmRc zJ6mo+17izrDAp/l5pzYTFoLC1IKQogWzpCYHY1Cvi2aJqKP5e31KxCfsEmh2pyNa5ac Bm8AvtW+gEf/ZJhTdLoN3N32kT2P2ZFrNhgiRgdLayYYrktwNv5wtZr7laxbCe5HicwB sYR01qGudYsWXpzEpyuk8OqF/AdThgYgFs2prs5vaYTdwMzBhwPTmtaUNt4ln+M0tFZF DCpQ== X-Gm-Message-State: AN3rC/7QVvFxsHxRPUnCv0n4Gib+cFvYuxN2KQiAa0QxTRCutRnaXYls 2TsmkLOVANVla5qsxHrA/Q== X-Received: by 10.28.153.140 with SMTP id b134mr2143406wme.124.1491427881955; Wed, 05 Apr 2017 14:31:21 -0700 (PDT) Received: from [192.168.0.100] ([37.189.166.198]) by smtp.gmail.com with ESMTPSA id d4sm8185670wrb.24.2017.04.05.14.31.20 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 05 Apr 2017 14:31:20 -0700 (PDT) Subject: Re: [PATCH 2/2] Class-ify ptid_t To: Simon Marchi References: <20170404183235.10589-1-simon.marchi@ericsson.com> <20170404183235.10589-2-simon.marchi@ericsson.com> <580e9a8a-d59b-095c-cf56-ee2f50fe46df@redhat.com> <84b33a5c655af3f344494c9e9ee473d6@polymtl.ca> Cc: Simon Marchi , gdb-patches@sourceware.org From: Pedro Alves Message-ID: Date: Wed, 05 Apr 2017 21:31:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <84b33a5c655af3f344494c9e9ee473d6@polymtl.ca> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-SW-Source: 2017-04/txt/msg00132.txt.bz2 On 04/05/2017 08:44 PM, Simon Marchi wrote: > On 2017-04-05 11:47, Pedro Alves wrote: >> Hi Simon, >> >> Hmm, "unit tests or it didn't happen" ? :-) > > Right, I don't have the unit test in GDB mindset yet. But of course, > it's a good idea, I'll do it. > >> On 04/04/2017 07:32 PM, Simon Marchi wrote: >>> I grew a bit tired of using ptid_get_{lwp,pid,tid} and friends, so I >>> decided to make it a bit easier to use by making it a proper class. >>> >>> Because ptid_t is used in things that aren't constructed, it is not >>> possible to have a constructor. Instead I added a "build" static >>> method, which maps well to the current ptid_build anyway, and ptid_t is >>> basically just a plain old data type with read-only methods. The >>> difference with before is that the fields are private, so it's not >>> possible to change a ptid_t field by mistake. >>> >>> The new methods of ptid_t map to existing functions/practice like this: >>> >>> ptid_t::build (pid, lwp, tid) -> ptid_build (pid, lwp, tid) >>> ptid_t::build (pid) -> pid_to_ptid (pid) >> >> Not sure these two are an improvement. pid_to_ptid is the >> counterpart of ptid_is_pid, and that is lost with the >> overloading of ptid_t::build. > > Would you prefer having a ptid_t::from_pid method instead? It would be > the counter part of ptid_t::is_pid. Or do you prefer if we keep the > current function? Either of those sounds fine. Or maybe that's not a big deal, and a ctor that takes a single "pid" would be fine. See below. >>> Also, I'm not sure if it's worth it (because of ptid_t's relatively >>> small size), but I have made the functions and methods take ptid_t >>> arguments by const reference instead of by value. >> >> I'd guess that the structure is still sufficiently small that passing >> by value would be a benefit (plus, it avoids inefficiency caused >> by the compiler having to assume that the references can alias), >> but OTOH, this structure is likely to grow with the multi-target >> work. Fine with me to go with what you have. > > Ok. Yeah, also, the compiler will probably be able to inline these most of the times. > >>> >>> /* See ptid.h for these. */ >>> >>> -ptid_t null_ptid = { 0, 0, 0 }; >>> -ptid_t minus_one_ptid = { -1, 0, 0 }; >>> +ptid_t null_ptid = ptid_t::build (0, 0, 0); >>> +ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0); >> >> It's probably going to be worth it to sprinkle "constexpr" >> all over the new API. Helps with static_asserts in >> unit testing too. *cough* :-) > > Ok, will look into it. Thanks. constexpr in C++11 forces you to write a single return statement (C++14 gets rid of that requirement), but it looks quite doable. Also, note that it's not true that this type can't have a constructor. It can, as long as the type remains POD. I've pasted below a quick patch on top of yours to get you going. Note the static_asserts. >> Note that C99 designated initializers are not valid C++11. >> Not sure whether any compiler _doesn't_ support them though. > > Ok. But anyway C++11-style initialization is probably better anyway. > Is the following ok? > > struct thread_resume default_action { null_ptid }; ISTR that in C++11 you'll need the double "{{" levels, like: thread_resume default_action {{null_ptid}}; and that C++14 removed that requirement (brace elision). But I haven't confirmed. Whatever works with -std=c++11. >From 63f3c4ed145a069df68991fabbf025ffaedf8cf6 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 5 Apr 2017 22:21:58 +0100 Subject: [PATCH] POD --- gdb/common/ptid.c | 8 +++---- gdb/common/ptid.h | 68 ++++++++++++++++++++++++++++--------------------------- 2 files changed, 39 insertions(+), 37 deletions(-) diff --git a/gdb/common/ptid.c b/gdb/common/ptid.c index ca51a4e..dff0071 100644 --- a/gdb/common/ptid.c +++ b/gdb/common/ptid.c @@ -22,8 +22,8 @@ /* See ptid.h for these. */ -ptid_t null_ptid = ptid_t::build (0, 0, 0); -ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0); +ptid_t null_ptid = ptid_t (0, 0, 0); +ptid_t minus_one_ptid = ptid_t (-1, 0, 0); /* See ptid.h. */ @@ -31,7 +31,7 @@ ptid_t minus_one_ptid = ptid_t::build (-1, 0, 0); ptid_t ptid_build (int pid, long lwp, long tid) { - return ptid_t::build (pid, lwp, tid); + return ptid_t (pid, lwp, tid); } /* See ptid.h. */ @@ -39,7 +39,7 @@ ptid_build (int pid, long lwp, long tid) ptid_t pid_to_ptid (int pid) { - return ptid_t::build (pid); + return ptid_t (pid); } /* See ptid.h. */ diff --git a/gdb/common/ptid.h b/gdb/common/ptid.h index c8649ae..1cb57e1 100644 --- a/gdb/common/ptid.h +++ b/gdb/common/ptid.h @@ -20,6 +20,8 @@ #ifndef PTID_H #define PTID_H +#include + class ptid_t; /* The null or zero ptid, often used to indicate no process. */ @@ -44,69 +46,65 @@ extern ptid_t minus_one_ptid; class ptid_t { public: - static ptid_t build (int pid, long lwp = 0, long tid = 0) - { - ptid_t ptid; + /* Must have a trivial defaulted default constructor so that the + type remains POD. */ + ptid_t () noexcept = default; - ptid.m_pid = pid; - ptid.m_lwp = lwp; - ptid.m_tid = tid; + constexpr ptid_t (int pid, long lwp = 0, long tid = 0) + : m_pid (pid), m_lwp (lwp), m_tid (tid) + {} - return ptid; - } - - bool is_pid () const + constexpr bool is_pid () const { - if (is_any () || is_null()) - return false; - - return m_lwp == 0 && m_tid == 0; + return (!is_any () + && !is_null() + && m_lwp == 0 + && m_tid == 0); } - bool is_null () const + constexpr bool is_null () const { return *this == null_ptid; } - bool is_any () const + constexpr bool is_any () const { return *this == minus_one_ptid; } - int pid () const + constexpr int pid () const { return m_pid; } - bool lwp_p () const + constexpr bool lwp_p () const { return m_lwp != 0; } - long lwp () const + constexpr long lwp () const { return m_lwp; } - bool tid_p () const + constexpr bool tid_p () const { return m_tid != 0; } - long tid () const + constexpr long tid () const { return m_tid; } - bool operator== (const ptid_t &other) const + constexpr bool operator== (const ptid_t &other) const { return (m_pid == other.m_pid && m_lwp == other.m_lwp && m_tid == other.m_tid); } - bool matches (const ptid_t &filter) const + constexpr bool matches (const ptid_t &filter) const { - /* If filter represents any ptid, it's always a match. */ - if (filter.is_any ()) - return true; - - /* If filter is only a pid, any ptid with that pid matches. */ - if (filter.is_pid () && m_pid == filter.pid ()) - return true; - - /* Otherwise, this ptid only matches if it's exactly equal to filter. */ - return *this == filter; + return (/* If filter represents any ptid, it's always a match. */ + filter.is_any () + /* 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); } private: @@ -120,6 +118,10 @@ private: long m_tid; }; +static_assert (std::is_pod::value, ""); + +static_assert (ptid_t(1).pid () == 1, ""); + /* Make a ptid given the necessary PID, LWP, and TID components. */ ptid_t ptid_build (int pid, long lwp, long tid); -- 2.5.5