Patchwork gdb/linux-fork: simplify one_fork_p

login
register
mail settings
Submitter Simon Marchi
Date Jan. 19, 2020, 4:10 p.m.
Message ID <20200119161058.864-1-simon.marchi@polymtl.ca>
Download mbox | patch
Permalink /patch/37441/
State New
Headers show

Comments

Simon Marchi - Jan. 19, 2020, 4:10 p.m.
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(-)
Doug Evans via gdb-patches - Jan. 19, 2020, 4:41 p.m.
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
>
Simon Marchi - Jan. 19, 2020, 4:53 p.m.
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
Doug Evans via gdb-patches - Jan. 19, 2020, 4:56 p.m.
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
Pedro Alves - Jan. 20, 2020, 3:05 p.m.
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

Patch

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.  */