Message ID | 20200119161058.864-1-simon.marchi@polymtl.ca |
---|---|
State | New, archived |
Headers |
Received: (qmail 65004 invoked by alias); 19 Jan 2020 16:11:03 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <gdb-patches.sourceware.org> List-Unsubscribe: <mailto:gdb-patches-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:gdb-patches-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/gdb-patches/> List-Post: <mailto:gdb-patches@sourceware.org> List-Help: <mailto:gdb-patches-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 64996 invoked by uid 89); 19 Jan 2020 16:11:03 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-18.7 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_SOFTFAIL autolearn=ham version=3.3.1 spammy=HContent-Transfer-Encoding:8bit X-HELO: barracuda.ebox.ca Received: from barracuda.ebox.ca (HELO barracuda.ebox.ca) (96.127.255.19) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Sun, 19 Jan 2020 16:11:02 +0000 Received: from smtp.ebox.ca (smtp.ebox.ca [96.127.255.82]) by barracuda.ebox.ca with ESMTP id 5WL4h78QTfFAfCbG (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 19 Jan 2020 11:11:00 -0500 (EST) Received: from simark.lan (unknown [192.222.164.54]) by smtp.ebox.ca (Postfix) with ESMTP id E7076441B21; Sun, 19 Jan 2020 11:10:59 -0500 (EST) From: Simon Marchi <simon.marchi@polymtl.ca> To: gdb-patches@sourceware.org Cc: Simon Marchi <simon.marchi@polymtl.ca> Subject: [PATCH] gdb/linux-fork: simplify one_fork_p Date: Sun, 19 Jan 2020 11:10:58 -0500 Message-Id: <20200119161058.864-1-simon.marchi@polymtl.ca> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes |
Commit Message
Simon Marchi
Jan. 19, 2020, 4:10 p.m. UTC
Unless I'm missing something, this function is a complicated way of saying "fork_list.size () == 1". gdb/ChangeLog: * linux-fork.c (one_fork_p): Simplify. --- gdb/linux-fork.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Comments
On Sun, Jan 19, 2020 at 11:11 AM Simon Marchi <simon.marchi@polymtl.ca> wrote: > > Unless I'm missing something, this function is a complicated way of > saying "fork_list.size () == 1". Before C++11, size() wasn't guaranteed to run in constant time, so I assume the code was written to handle that. But GDB uses C++11, so this change seems fine. https://en.cppreference.com/w/cpp/container/list/size > > gdb/ChangeLog: > > * linux-fork.c (one_fork_p): Simplify. > --- > gdb/linux-fork.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c > index 284f1985d0dc..357188685d07 100644 > --- a/gdb/linux-fork.c > +++ b/gdb/linux-fork.c > @@ -110,8 +110,7 @@ find_last_fork (void) > static bool > one_fork_p () > { > - return (!fork_list.empty () > - && &fork_list.front () == &fork_list.back ()); > + return fork_list.size () == 1; > } > > /* Add a new fork to the internal fork list. */ > -- > 2.24.1 >
On 2020-01-19 11:41 a.m., Christian Biesinger wrote: > On Sun, Jan 19, 2020 at 11:11 AM Simon Marchi <simon.marchi@polymtl.ca> wrote: >> >> Unless I'm missing something, this function is a complicated way of >> saying "fork_list.size () == 1". > > Before C++11, size() wasn't guaranteed to run in constant time, so I > assume the code was written to handle that. But GDB uses C++11, so > this change seems fine. > https://en.cppreference.com/w/cpp/container/list/size Ahh, good point. Although by the time that change was made, we were already using C++11. I don't remember if we had a C++ < 11 phase, but if we did it was very short. Thanks for looking at it, I'll push it now. Simon
On Sun, Jan 19, 2020 at 11:53 AM Simon Marchi <simon.marchi@polymtl.ca> wrote: > > On 2020-01-19 11:41 a.m., Christian Biesinger wrote: > > On Sun, Jan 19, 2020 at 11:11 AM Simon Marchi <simon.marchi@polymtl.ca> wrote: > >> > >> Unless I'm missing something, this function is a complicated way of > >> saying "fork_list.size () == 1". > > > > Before C++11, size() wasn't guaranteed to run in constant time, so I > > assume the code was written to handle that. But GDB uses C++11, so > > this change seems fine. > > https://en.cppreference.com/w/cpp/container/list/size > > Ahh, good point. Although by the time that change was made, we were already > using C++11. I don't remember if we had a C++ < 11 phase, but if we did it > was very short. > > Thanks for looking at it, I'll push it now. Ah. it's also possible that whoever wrote the code just assumed that size() would run in linear time, of course. Christian
On 1/19/20 4:56 PM, Christian Biesinger via gdb-patches wrote: > On Sun, Jan 19, 2020 at 11:53 AM Simon Marchi <simon.marchi@polymtl.ca> wrote: >> >> On 2020-01-19 11:41 a.m., Christian Biesinger wrote: >>> On Sun, Jan 19, 2020 at 11:11 AM Simon Marchi <simon.marchi@polymtl.ca> wrote: >>>> >>>> Unless I'm missing something, this function is a complicated way of >>>> saying "fork_list.size () == 1". >>> >>> Before C++11, size() wasn't guaranteed to run in constant time, so I >>> assume the code was written to handle that. But GDB uses C++11, so >>> this change seems fine. >>> https://en.cppreference.com/w/cpp/container/list/size >> >> Ahh, good point. Although by the time that change was made, we were already >> using C++11. I don't remember if we had a C++ < 11 phase, but if we did it >> was very short. Yes, we had one. It was short. Everyone hated my unique_ptr emulation so much that we moved quickly to C++11. :-D >> >> Thanks for looking at it, I'll push it now. > > Ah. it's also possible that whoever wrote the code just assumed that > size() would run in linear time, of course. Note, I believe that size() isn't linear when compiled with gcc 4.8, since the new C++11 ABI was only introduced in GCC 5: https://developers.redhat.com/blog/2015/02/05/gcc5-and-the-c11-abi/ But I think it's OK to ignore that. Especially for non-hot code like here. I think it's reasonable to say that if you care about performance, you'll want to compile with a newer compiler. I was the one who wrote it (06974e6c05556e), but I don't remember why I did it that way. Might have been the non-O(1) issue, or it could have been about blindly C++-fying code without realizing the potential simplification. I agree that size () == 1 works just as well, assuming C++11 std::list. Thanks, Pedro Alves
diff --git a/gdb/linux-fork.c b/gdb/linux-fork.c index 284f1985d0dc..357188685d07 100644 --- a/gdb/linux-fork.c +++ b/gdb/linux-fork.c @@ -110,8 +110,7 @@ find_last_fork (void) static bool one_fork_p () { - return (!fork_list.empty () - && &fork_list.front () == &fork_list.back ()); + return fork_list.size () == 1; } /* Add a new fork to the internal fork list. */