[PATCH] Replace sort_tu_by_abbrev_offset with operator<

[PATCH] Replace sort_tu_by_abbrev_offset with operator<

[Tom Tromey]

I noticed that sort_tu_by_abbrev_offset only has a single caller.  It
seemed simpler to replace it with an implementation of operator<
instead.

gdb/ChangeLog
2021-05-15  Tom Tromey  <tom@tromey.com>

	* dwarf2/read.c (tu_abbrev_offset::operator<): New method.
	(sort_tu_by_abbrev_offset): Remove.
	(build_type_psymtabs): Update.
---
 gdb/ChangeLog     |  6 ++++++
 gdb/dwarf2/read.c | 18 +++++++-----------
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ac786abf6d2..f1bb9b2f3df 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -7351,19 +7351,16 @@ struct tu_abbrev_offset
   : sig_type (sig_type_), abbrev_offset (abbrev_offset_)
   {}
 
+  /* This is used when sorting.  */
+  bool operator< (const tu_abbrev_offset &other)
+  {
+    return abbrev_offset < other.abbrev_offset;
+  }
+
   signatured_type *sig_type;
   sect_offset abbrev_offset;
 };
 
-/* Helper routine for build_type_psymtabs, passed to std::sort.  */
-
-static bool
-sort_tu_by_abbrev_offset (const struct tu_abbrev_offset &a,
-			  const struct tu_abbrev_offset &b)
-{
-  return a.abbrev_offset < b.abbrev_offset;
-}
-
 /* Efficiently read all the type units.
 
    The efficiency is because we sort TUs by the abbrev table they use and
@@ -7431,8 +7428,7 @@ build_type_psymtabs (dwarf2_per_objfile *per_objfile)
 	}
     }
 
-  std::sort (sorted_by_abbrev.begin (), sorted_by_abbrev.end (),
-	     sort_tu_by_abbrev_offset);
+  std::sort (sorted_by_abbrev.begin (), sorted_by_abbrev.end ());
 
   abbrev_offset = (sect_offset) ~(unsigned) 0;
 
-- 
2.26.3

Re: [PATCH] Replace sort_tu_by_abbrev_offset with operator<

[Simon Marchi]

On 2021-05-15 11:19 a.m., Tom Tromey wrote:
> I noticed that sort_tu_by_abbrev_offset only has a single caller.  It
> seemed simpler to replace it with an implementation of operator<
> instead.
> 
> gdb/ChangeLog
> 2021-05-15  Tom Tromey  <tom@tromey.com>
> 
> 	* dwarf2/read.c (tu_abbrev_offset::operator<): New method.
> 	(sort_tu_by_abbrev_offset): Remove.
> 	(build_type_psymtabs): Update.
> ---
>  gdb/ChangeLog     |  6 ++++++
>  gdb/dwarf2/read.c | 18 +++++++-----------
>  2 files changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index ac786abf6d2..f1bb9b2f3df 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -7351,19 +7351,16 @@ struct tu_abbrev_offset
>    : sig_type (sig_type_), abbrev_offset (abbrev_offset_)
>    {}
>  
> +  /* This is used when sorting.  */
> +  bool operator< (const tu_abbrev_offset &other)
> +  {
> +    return abbrev_offset < other.abbrev_offset;
> +  }
> +
>    signatured_type *sig_type;
>    sect_offset abbrev_offset;
>  };
>  
> -/* Helper routine for build_type_psymtabs, passed to std::sort.  */
> -
> -static bool
> -sort_tu_by_abbrev_offset (const struct tu_abbrev_offset &a,
> -			  const struct tu_abbrev_offset &b)
> -{
> -  return a.abbrev_offset < b.abbrev_offset;
> -}
> -
>  /* Efficiently read all the type units.
>  
>     The efficiency is because we sort TUs by the abbrev table they use and
> @@ -7431,8 +7428,7 @@ build_type_psymtabs (dwarf2_per_objfile *per_objfile)
>  	}
>      }
>  
> -  std::sort (sorted_by_abbrev.begin (), sorted_by_abbrev.end (),
> -	     sort_tu_by_abbrev_offset);
> +  std::sort (sorted_by_abbrev.begin (), sorted_by_abbrev.end ());
>  
>    abbrev_offset = (sect_offset) ~(unsigned) 0;
>  
> 

LGTM, thanks.

Simon

Re: [PATCH] Replace sort_tu_by_abbrev_offset with operator<

[Vaseeharan Vinayagamoorthy]

I am seeing this error: passing ‘const tu_abbrev_offset’ as ‘this’ argument of ‘bool tu_abbrev_offset::operator<(const tu_abbrev_offset&)’ discards qualifiers [-fpermissive]
  

In file included from /usr/include/c++/4.8.2/algorithm:62:0,
                 from /src/binutils-gdb--gdb/gdb/dwarf2/read.c:85:
/usr/include/c++/4.8.2/bits/stl_algo.h: In instantiation of ‘_RandomAccessIterator std::__unguarded_partition(_RandomAccessIterator, _RandomAccessIterator, const _Tp&) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<tu_abbrev_offset*, std::vector<tu_abbrev_offset> >; _Tp = tu_abbrev_offset]’:
/usr/include/c++/4.8.2/bits/stl_algo.h:2283:70:   required from ‘_RandomAccessIterator std::__unguarded_partition_pivot(_RandomAccessIterator, _RandomAccessIterator) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<tu_abbrev_offset*, std::vector<tu_abbrev_offset> >]’
/usr/include/c++/4.8.2/bits/stl_algo.h:2315:54:   required from ‘void std::__introsort_loop(_RandomAccessIterator, _RandomAccessIterator, _Size) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<tu_abbrev_offset*, std::vector<tu_abbrev_offset> >; _Size = long int]’
/usr/include/c++/4.8.2/bits/stl_algo.h:5461:36:   required from ‘void std::sort(_RAIter, _RAIter) [with _RAIter = __gnu_cxx::__normal_iterator<tu_abbrev_offset*, std::vector<tu_abbrev_offset> >]’
/src/binutils-gdb--gdb/gdb/dwarf2/read.c:7195:64:   required from here
/usr/include/c++/4.8.2/bits/stl_algo.h:2245:19: error: passing ‘const tu_abbrev_offset’ as ‘this’ argument of ‘bool tu_abbrev_offset::operator<(const tu_abbrev_offset&)’ discards qualifiers [-fpermissive]
    while (__pivot < *__last)


The build/host/target setup is:
Build: x86_64 (Linux)
Host: : x86_64 (Linux)
Target: arm-none-eabi / arm-none-linux-gnueabi  / aarch64-none-elf / aarch64-none-linux-gnu


Kind regards
Vasee


On 15/05/2021, 16:19, "Gdb-patches on behalf of Tom Tromey" <gdb-patches-bounces@sourceware.org on behalf of tom@tromey.com> wrote:

    I noticed that sort_tu_by_abbrev_offset only has a single caller.  It
    seemed simpler to replace it with an implementation of operator<
    instead.

    gdb/ChangeLog
    2021-05-15  Tom Tromey  <tom@tromey.com>

    	* dwarf2/read.c (tu_abbrev_offset::operator<): New method.
    	(sort_tu_by_abbrev_offset): Remove.
    	(build_type_psymtabs): Update.
    ---
     gdb/ChangeLog     |  6 ++++++
     gdb/dwarf2/read.c | 18 +++++++-----------
     2 files changed, 13 insertions(+), 11 deletions(-)

    diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
    index ac786abf6d2..f1bb9b2f3df 100644
    --- a/gdb/dwarf2/read.c
    +++ b/gdb/dwarf2/read.c
    @@ -7351,19 +7351,16 @@ struct tu_abbrev_offset
       : sig_type (sig_type_), abbrev_offset (abbrev_offset_)
       {}

    +  /* This is used when sorting.  */
    +  bool operator< (const tu_abbrev_offset &other)
    +  {
    +    return abbrev_offset < other.abbrev_offset;
    +  }
    +
       signatured_type *sig_type;
       sect_offset abbrev_offset;
     };

    -/* Helper routine for build_type_psymtabs, passed to std::sort.  */
    -
    -static bool
    -sort_tu_by_abbrev_offset (const struct tu_abbrev_offset &a,
    -			  const struct tu_abbrev_offset &b)
    -{
    -  return a.abbrev_offset < b.abbrev_offset;
    -}
    -
     /* Efficiently read all the type units.

        The efficiency is because we sort TUs by the abbrev table they use and
    @@ -7431,8 +7428,7 @@ build_type_psymtabs (dwarf2_per_objfile *per_objfile)
     	}
         }

    -  std::sort (sorted_by_abbrev.begin (), sorted_by_abbrev.end (),
    -	     sort_tu_by_abbrev_offset);
    +  std::sort (sorted_by_abbrev.begin (), sorted_by_abbrev.end ());

       abbrev_offset = (sect_offset) ~(unsigned) 0;

    -- 
    2.26.3

Re: [PATCH] Replace sort_tu_by_abbrev_offset with operator<

[John Baldwin]

On 5/19/21 3:22 PM, Vaseeharan Vinayagamoorthy via Gdb-patches wrote:
> I am seeing this error: passing ‘const tu_abbrev_offset’ as ‘this’ argument of ‘bool tu_abbrev_offset::operator<(const tu_abbrev_offset&)’ discards qualifiers [-fpermissive]
>    
> 
> In file included from /usr/include/c++/4.8.2/algorithm:62:0,
>                   from /src/binutils-gdb--gdb/gdb/dwarf2/read.c:85:
> /usr/include/c++/4.8.2/bits/stl_algo.h: In instantiation of ‘_RandomAccessIterator std::__unguarded_partition(_RandomAccessIterator, _RandomAccessIterator, const _Tp&) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<tu_abbrev_offset*, std::vector<tu_abbrev_offset> >; _Tp = tu_abbrev_offset]’:
> /usr/include/c++/4.8.2/bits/stl_algo.h:2283:70:   required from ‘_RandomAccessIterator std::__unguarded_partition_pivot(_RandomAccessIterator, _RandomAccessIterator) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<tu_abbrev_offset*, std::vector<tu_abbrev_offset> >]’
> /usr/include/c++/4.8.2/bits/stl_algo.h:2315:54:   required from ‘void std::__introsort_loop(_RandomAccessIterator, _RandomAccessIterator, _Size) [with _RandomAccessIterator = __gnu_cxx::__normal_iterator<tu_abbrev_offset*, std::vector<tu_abbrev_offset> >; _Size = long int]’
> /usr/include/c++/4.8.2/bits/stl_algo.h:5461:36:   required from ‘void std::sort(_RAIter, _RAIter) [with _RAIter = __gnu_cxx::__normal_iterator<tu_abbrev_offset*, std::vector<tu_abbrev_offset> >]’
> /src/binutils-gdb--gdb/gdb/dwarf2/read.c:7195:64:   required from here
> /usr/include/c++/4.8.2/bits/stl_algo.h:2245:19: error: passing ‘const tu_abbrev_offset’ as ‘this’ argument of ‘bool tu_abbrev_offset::operator<(const tu_abbrev_offset&)’ discards qualifiers [-fpermissive]
>      while (__pivot < *__last)

I might have just pushed a fix for this.  clang wasn't happy about the method not being
const and this seems like it might be a variant of the same (the method being non-const
might have had the effect of this error of removing const from 'this').

-- 
John Baldwin