From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from lndn.lancelotsix.com (lndn.lancelotsix.com [51.195.220.111]) by sourceware.org (Postfix) with ESMTPS id 492B2385742E for ; Tue, 17 May 2022 08:43:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 492B2385742E Received: from ubuntu.lan (unknown [IPv6:2a02:390:9086::635]) by lndn.lancelotsix.com (Postfix) with ESMTPSA id 5E9BB86E09; Tue, 17 May 2022 08:43:09 +0000 (UTC) Date: Tue, 17 May 2022 08:43:00 +0000 From: Lancelot SIX To: Tom Tromey Cc: gdb-patches@sourceware.org Subject: Re: [PATCH] Finalize each cooked index separately Message-ID: <20220517084248.j6w4du4pugvtzvyp@ubuntu.lan> References: <20220516184614.1171732-1-tromey@adacore.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220516184614.1171732-1-tromey@adacore.com> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.5.11 (lndn.lancelotsix.com [0.0.0.0]); Tue, 17 May 2022 08:43:09 +0000 (UTC) X-Spam-Status: No, score=-11.6 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 May 2022 08:43:14 -0000 Hi, I have a couple of comments/suggestions inlined in the patch below. > @@ -339,3 +248,98 @@ cooked_index_vector::finalize () > return *a < *b; > }); > } > + > +/* See cooked-index.h. */ > + > +cooked_index::range > +cooked_index::find (gdb::string_view name, bool completing) > +{ > + wait (); > + > + auto lower = std::lower_bound (m_entries.begin (), m_entries.end (), > + name, > + [=] (const cooked_index_entry *entry, > + const gdb::string_view &n) > + { > + int cmp = strncasecmp (entry->canonical, n.data (), n.length ()); > + if (cmp != 0 || completing) > + return cmp < 0; > + return strlen (entry->canonical) < n.length (); > + }); > + > + auto upper = std::upper_bound (m_entries.begin (), m_entries.end (), > + name, > + [=] (const gdb::string_view &n, > + const cooked_index_entry *entry) > + { > + int cmp = strncasecmp (n.data (), entry->canonical, n.length ()); > + if (cmp != 0 || completing) > + return cmp < 0; > + return n.length () < strlen (entry->canonical); > + }); > + > + return range (lower, upper); > +} > + > +cooked_index_vector::cooked_index_vector (vec_type &&vec) > + : m_vector (std::move (vec)) > +{ > + for (auto &idx : m_vector) > + idx->finalize (); > +} > + > +/* See cooked-index.h. */ > + > +dwarf2_per_cu_data * > +cooked_index_vector::lookup (CORE_ADDR addr) > +{ > + for (const auto &index : m_vector) > + { > + dwarf2_per_cu_data *result = index->lookup (addr); > + if (result != nullptr) > + return result; > + } > + return nullptr; > +} > + > +/* See cooked-index.h. */ > + > +std::vector > +cooked_index_vector::get_addrmaps () > +{ > + std::vector result; > + for (const auto &index : m_vector) > + result.push_back (index->m_addrmap); > + return result; > +} > + > +/* See cooked-index.h. */ > + > +cooked_index_vector::range > +cooked_index_vector::find (gdb::string_view name, bool completing) > +{ > + std::vector result_range; Since the result_range's expected size is already known, it is possible to pre-allocate the associated storage with result_range.reserve (m_vector.size); > + for (auto &entry : m_vector) > + result_range.push_back (entry->find (name, completing)); > + return range (std::move (result_range)); > +} > + > +/* See cooked-index.h. */ > + > +const cooked_index_entry * > +cooked_index_vector::get_main () const > +{ > + const cooked_index_entry *result = nullptr; > + > + for (const auto &index : m_vector) > + { > + const cooked_index_entry *entry = index->get_main (); > + if (result == nullptr > + || ((result->flags & IS_MAIN) == 0 > + && entry != nullptr > + && (entry->flags & IS_MAIN) != 0)) > + result = entry; > + } > + > + return result; > +} > diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h > index ad4de05ba52..3b83250ef0f 100644 > --- a/gdb/dwarf2/cooked-index.h > +++ b/gdb/dwarf2/cooked-index.h > @@ -273,8 +318,10 @@ class cooked_index_vector : public dwarf_scanner_base > /* Return a range of all the entries. */ > range all_entries () > { > - m_future.wait (); > - return { m_entries.begin (), m_entries.end () }; > + std::vector result_range; Similar here, reserve can come in handy. > + for (auto &entry : m_vector) > + result_range.push_back (entry->all_entries ()); > + return range (std::move (result_range)); > } > > /* Look up ADDR in the address map, and return either the > diff --git a/gdbsupport/range-chain.h b/gdbsupport/range-chain.h > new file mode 100644 > index 00000000000..9bd6eb19480 > --- /dev/null > +++ b/gdbsupport/range-chain.h > @@ -0,0 +1,121 @@ > +/* A range adapter that wraps multiple ranges > + Copyright (C) 2022 Free Software Foundation, Inc. > + > + This file is part of GDB. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; either version 3 of the License, or > + (at your option) any later version. > + > + This program is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + GNU General Public License for more details. > + > + You should have received a copy of the GNU General Public License > + along with this program. If not, see . */ > + > +#ifndef GDBSUPPORT_RANGE_CHAIN_H > +#define GDBSUPPORT_RANGE_CHAIN_H > + > +/* A range adapter that presents a number of ranges as if it were a > + single range. That is, iterating over a range_chain will iterate > + over each sub-range in order. */ > +template > +struct range_chain > +{ > + /* The type of the iterator that is created by this range. */ > + class iterator > + { > + public: > + > + iterator (std::vector &ranges, size_t idx) > + : m_index (idx), > + m_ranges (ranges) > + { > + skip_empty (); > + } > + > + bool operator== (const iterator &other) const > + { > + if (m_index != other.m_index || &m_ranges != &other.m_ranges) > + return false; > + if (m_current.has_value () != other.m_current.has_value ()) > + return false; > + if (m_current.has_value ()) > + return *m_current == *other.m_current; > + return true; > + } > + > + bool operator!= (const iterator &other) const > + { > + return !(*this == other); > + } > + > + iterator &operator++ () > + { > + ++*m_current; > + if (*m_current == m_ranges[m_index].end ()) > + { > + ++m_index; > + skip_empty (); > + } > + return *this; > + } > + > + typename Range::iterator::value_type operator* () const > + { > + return **m_current; > + } > + > + private: > + /* Skip empty sub-ranges. If this finds a valid sub-range, > + m_current is updated to point to its start; otherwise, > + m_current is reset. */ > + void skip_empty () > + { > + for (; m_index < m_ranges.size (); ++m_index) > + { > + m_current = m_ranges[m_index].begin (); > + if (*m_current != m_ranges[m_index].end ()) > + return; > + } > + m_current.reset (); > + } > + > + /* Index into the vector indicating where the current iterator > + comes from. */ > + size_t m_index; > + /* The current iterator into one of the vector ranges. If no > + value then this (outer) iterator is at the end of the overall > + range. */ > + gdb::optional m_current; > + /* Vector of ranges. */ > + std::vector &m_ranges; > + }; > + > + /* Create a new range_chain, transferring in a vector of > + sub-ranges. */ > + range_chain (std::vector &&ranges) > + : m_ranges (ranges) Did you forget the std::move (ranges) here? Otherwise I think m_ranges is initialized using a copy ctor instead of the intended move ctor. Also, I am not certain range_chain should be restricted to only take ownership of the vector or ranges. This version should allow to take lavlue or rvalue ref depending on the caller: template range_chain (Ranges &&ranges) : m_ranges (std::forward (ranges)) { } That being said, it is not needed in your patch and if someone needs this it will be easy enough to add at that time. > + { > + } > + > + iterator begin () I was wondering why the begin () and end () are not const. Turns out that they could become const if the following adjustments are done to the iterator class: - The range_chain::iterator::m_vector becomes a const member - The range_chain::iterator::iterator ctor takes its vector parameter as a const ref. I do not think there is case here for the iterator to change the underlying vector, so making this const enforces it. WDYT? > + { > + return iterator (m_ranges, 0); > + } > + > + iterator end () > + { > + return iterator (m_ranges, m_ranges.size ()); > + } > + > +private: > + > + /* The sub-ranges. */ > + std::vector m_ranges; > +}; > + > +#endif /* GDBSUPPORT_RANGE_CHAIN_H */ > -- > 2.34.1 > Best, Lancelot.