[PATCH] Finalize each cooked index separately
Lancelot SIX
lsix@lancelotsix.com
Tue May 17 08:43:00 GMT 2022
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<addrmap *>
> +cooked_index_vector::get_addrmaps ()
> +{
> + std::vector<addrmap *> 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<cooked_index::range> 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<cooked_index::range> 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 <http://www.gnu.org/licenses/>. */
> +
> +#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<typename Range>
> +struct range_chain
> +{
> + /* The type of the iterator that is created by this range. */
> + class iterator
> + {
> + public:
> +
> + iterator (std::vector<Range> &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<typename Range::iterator> m_current;
> + /* Vector of ranges. */
> + std::vector<Range> &m_ranges;
> + };
> +
> + /* Create a new range_chain, transferring in a vector of
> + sub-ranges. */
> + range_chain (std::vector<Range> &&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<typename Ranges>
range_chain (Ranges &&ranges)
: m_ranges (std::forward<Ranges> (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<Range> m_ranges;
> +};
> +
> +#endif /* GDBSUPPORT_RANGE_CHAIN_H */
> --
> 2.34.1
>
Best,
Lancelot.
More information about the Gdb-patches
mailing list