From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22666 invoked by alias); 9 Nov 2018 16:52:54 -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 22545 invoked by uid 89); 9 Nov 2018 16:52:53 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.7 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_2,GIT_PATCH_3,SPF_HELO_PASS,SPF_SOFTFAIL autolearn=ham version=3.3.2 spammy=HTo:U*palves X-HELO: mail.baldwin.cx Received: from bigwig.baldwin.cx (HELO mail.baldwin.cx) (96.47.65.170) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 09 Nov 2018 16:52:51 +0000 Received: from John-Baldwins-MacBook-Pro-2.local (ralph.baldwin.cx [66.234.199.215]) by mail.baldwin.cx (Postfix) with ESMTPSA id 7D9C810B429; Fri, 9 Nov 2018 11:52:49 -0500 (EST) Subject: Re: [PATCH v2 2/3] Add an optional "alias" attribute to syscall entries. To: Pedro Alves , gdb-patches@sourceware.org References: <20181106175431.59832-1-jhb@FreeBSD.org> <20181106175431.59832-3-jhb@FreeBSD.org> <2ab3f4b5-cc23-8b3d-dd55-66646cdffff1@redhat.com> From: John Baldwin Openpgp: preference=signencrypt Message-ID: <55380054-16e3-ef67-c105-beea11766860@FreeBSD.org> Date: Fri, 09 Nov 2018 16:52:00 -0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <2ab3f4b5-cc23-8b3d-dd55-66646cdffff1@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-11/txt/msg00161.txt.bz2 On 11/9/18 8:31 AM, Pedro Alves wrote: > Hi, > > On 11/06/2018 05:54 PM, John Baldwin wrote: > >> --- a/gdb/syscalls/gdb-syscalls.dtd >> +++ b/gdb/syscalls/gdb-syscalls.dtd >> @@ -12,4 +12,5 @@ >> > name CDATA #REQUIRED >> number CDATA #REQUIRED >> + alias CDATA #IMPLIED >> groups CDATA #IMPLIED> > > I think there should be a NEWS entry for this. Ok. > I'd think there should be a docs update as well, but it > looks like this dtd isn't documented in the manual. :-/ > > >> + /* We have a name. Let's check if it's valid and fetch a >> + list of matching numbers. */ >> + std::vector numbers = get_syscalls_by_name (gdbarch, cur_name); >> >> - if (s.number == UNKNOWN_SYSCALL) >> + if (numbers.empty ()) >> /* Here we have to issue an error instead of a warning, >> because GDB cannot do anything useful if there's no >> syscall number to be caught. */ >> error (_("Unknown syscall name '%s'."), cur_name); >> >> /* Ok, it's valid. */ >> - result.push_back (s.number); >> + for (int number : numbers) >> + result.push_back (number); > > Nit: this return-vector interface seems like leads to a bit of unnecessary > work, and double the number of necessary vector/heap allocations -- if > get_syscalls_by_name were passed RESULT instead of returning a new vector, > it could add to RESULT directly? The code above would like something like this > instead with get_syscalls_by_name returning true/false to indicate > whether something was added: > > /* We have a name. Let's check if it's valid and fetch a > list of matching numbers. */ > if (!get_syscalls_by_name (gdbarch, cur_name, result)) > /* Here we have to issue an error instead of a warning, > because GDB cannot do anything useful if there's no > syscall number to be caught. */ > error (_("Unknown syscall name '%s'."), cur_name); > > That means there's no need to copy numbers between the vectors > (since there's only one vector). > > Same for get_syscalls_by_group. Oh, yes, that is a much better idea. I will adopt that change in both patches. > Note that instead of: > >> + for (int number : numbers) >> + result.push_back (number); > > You could write: > > result.insert (result.end (), numbers.begin (), numbers.end ()); > > which may be more efficient assuming insert is smart enough > to call std::vector::reserve to allocate space in one go. Ok. I wasn't quite sure if there was a more efficient idiom to use. It's a bit of a shame std::vector<> doesn't have an explicit append/join method similar to list.append() in Python. -- John Baldwin                                                                            Â