From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5511 invoked by alias); 5 Apr 2017 19:44:36 -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 5237 invoked by uid 89); 5 Apr 2017 19:44:28 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-23.8 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy= 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; Wed, 05 Apr 2017 19:44:27 +0000 Received: by simark.ca (Postfix, from userid 33) id A77AA1E89F; Wed, 5 Apr 2017 15:44:25 -0400 (EDT) To: Pedro Alves Subject: Re: [PATCH 2/2] 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: Wed, 05 Apr 2017 19:44:00 -0000 From: Simon Marchi Cc: Simon Marchi , gdb-patches@sourceware.org In-Reply-To: <580e9a8a-d59b-095c-cf56-ee2f50fe46df@redhat.com> References: <20170404183235.10589-1-simon.marchi@ericsson.com> <20170404183235.10589-2-simon.marchi@ericsson.com> <580e9a8a-d59b-095c-cf56-ee2f50fe46df@redhat.com> Message-ID: <84b33a5c655af3f344494c9e9ee473d6@polymtl.ca> X-Sender: simon.marchi@polymtl.ca User-Agent: Roundcube Webmail/1.2.4 X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg00127.txt.bz2 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? >> ptid.is_pid () -> ptid_is_pid (ptid) >> ptid == other -> ptid_equal (ptid, other) >> ptid.is_null () -> ptid_equal (ptid, null_ptid) >> ptid.is_any () -> ptid_equal (ptid, minus_one_ptid) >> ptid.pid () -> ptid_get_pid (ptid) >> ptid.lwp_p () -> ptid_lwp_p (ptid) >> ptid.lwp () -> ptid_get_lwp (ptid) >> ptid.tid_p () -> ptid_tid_p (ptid) >> ptid.tid () -> ptid_get_tid (ptid) >> ptid.matches (filter) -> ptid_match (ptid, filter) >> >> I've replaced the implementation of the existing functions with calls >> to >> the new methods. People are encouraged to gradually switch to using >> the >> ptid_t methods instead of the functions (or we can change them all in >> one pass eventually). >> >> 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. >> >> /* 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. >> -struct ptid >> +class ptid_t >> { >> +public: >> + static ptid_t build (int pid, long lwp = 0, long tid = 0) >> + { >> + ptid_t ptid; >> + >> + ptid.m_pid = pid; >> + ptid.m_lwp = lwp; >> + ptid.m_tid = tid; >> + >> + return ptid; >> + } >> + >> + bool is_pid () const >> + { >> + if (is_any () || is_null()) > > Missing space after "null". Thanks, fixed > Wonder about migrating/copying the comments API comments to > the methods, if these are the entry points that people should > be looking at going forward. Right, it would make sense. >> + return false; >> + >> + return m_lwp == 0 && m_tid == 0; >> + } >> + > >> diff --git a/gdb/gdbserver/server.c b/gdb/gdbserver/server.c >> index 4bc7f71b00..1287114cc1 100644 >> --- a/gdb/gdbserver/server.c >> +++ b/gdb/gdbserver/server.c >> @@ -2654,7 +2654,9 @@ handle_v_cont (char *own_buf) >> char *p, *q; >> int n = 0, i = 0; >> struct thread_resume *resume_info; >> - struct thread_resume default_action = {{0}}; >> + struct thread_resume default_action = { >> + .thread = null_ptid, >> + }; > > 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 }; Thanks, Simon