From 3e771c2ad7b3b1ea48fc56f4a5b20f518973810a Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Wed, 30 Sep 2026 18:37:54 +0200 Subject: [PATCH] Deduplicate the linear key search in ordered_map emplace, at, erase(key), count and find each repeated the same "for (auto it = begin(); it != end(); ++it) if (m_compare(it->first, key)) ..." loop (15 copies across their key_type and transparent KeyType&& overloads), and both erase(key) overloads additionally repeated the exception-sensitive in-place reconstruction (destroy, placement-new, pop_back) used to remove an element while keeping the const Key non-movable. Add two private helpers: find_impl(Self&, KeyType&&), a static member template that runs the search once for either constness of the receiver, and erase_at(iterator), which keeps the existing pop_back-based reconstruction instead of switching to erase()/resize() (which would add a DefaultInsertable requirement). Route find, at, count, emplace, insert(const value_type&) and both erase(key) overloads through them. Same signatures, is_usable_as_key_type constraints and exception messages/types. Overlaps #5609 and #5685, which both rewrite emplace (#5609 also touches insert and adds private members at the end of the class); whichever of this commit and those PRs lands second will need a rebase. #5724 item 6 Signed-off-by: Niels Lohmann --- include/nlohmann/ordered_map.hpp | 183 ++++++++++++------------------- single_include/nlohmann/json.hpp | 183 ++++++++++++------------------- 2 files changed, 136 insertions(+), 230 deletions(-) diff --git a/include/nlohmann/ordered_map.hpp b/include/nlohmann/ordered_map.hpp index 7b8cf70f4..2b500e6ae 100644 --- a/include/nlohmann/ordered_map.hpp +++ b/include/nlohmann/ordered_map.hpp @@ -70,14 +70,41 @@ template , return *this; } +private: + /// @brief find the entry for @a key, for either constness of @a self + /// @note the single place that performs the linear key search + template + static auto find_impl(Self& self, KeyType&& key) -> decltype(self.begin()) + { + for (auto it = self.begin(); it != self.end(); ++it) + { + if (self.m_compare(it->first, key)) + { + return it; + } + } + return self.end(); + } + + /// @brief remove the entry @a it points to, preserving order + /// @note keys are not movable, so the tail is destroyed and re-constructed in place + void erase_at(iterator it) + { + for (auto next = it; ++next != this->end(); ++it) + { + it->~value_type(); // Destroy but keep allocation + new (&*it) value_type{std::move(*next)}; + } + Container::pop_back(); + } + +public: std::pair emplace(const key_type& key, T&& t) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it != this->end()) { - if (m_compare(it->first, key)) - { - return {it, false}; - } + return {it, false}; } Container::emplace_back(key, std::forward(t)); return {std::prev(this->end()), true}; @@ -87,12 +114,10 @@ template , detail::is_usable_as_key_type::value, int> = 0> std::pair emplace(KeyType && key, T && t) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it != this->end()) { - if (m_compare(it->first, key)) - { - return {it, false}; - } + return {it, false}; } Container::emplace_back(std::forward(key), std::forward(t)); return {std::prev(this->end()), true}; @@ -124,75 +149,55 @@ template , T& at(const key_type& key) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it == this->end()) { - if (m_compare(it->first, key)) - { - return it->second; - } + JSON_THROW(std::out_of_range("key not found")); } - - JSON_THROW(std::out_of_range("key not found")); + return it->second; } template::value, int> = 0> T & at(KeyType && key) // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it == this->end()) { - if (m_compare(it->first, key)) - { - return it->second; - } + JSON_THROW(std::out_of_range("key not found")); } - - JSON_THROW(std::out_of_range("key not found")); + return it->second; } const T& at(const key_type& key) const { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it == this->end()) { - if (m_compare(it->first, key)) - { - return it->second; - } + JSON_THROW(std::out_of_range("key not found")); } - - JSON_THROW(std::out_of_range("key not found")); + return it->second; } template::value, int> = 0> const T & at(KeyType && key) const // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it == this->end()) { - if (m_compare(it->first, key)) - { - return it->second; - } + JSON_THROW(std::out_of_range("key not found")); } - - JSON_THROW(std::out_of_range("key not found")); + return it->second; } size_type erase(const key_type& key) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it != this->end()) { - if (m_compare(it->first, key)) - { - // Since we cannot move const Keys, re-construct them in place - for (auto next = it; ++next != this->end(); ++it) - { - it->~value_type(); // Destroy but keep allocation - new (&*it) value_type{std::move(*next)}; - } - Container::pop_back(); - return 1; - } + erase_at(it); + return 1; } return 0; } @@ -201,19 +206,11 @@ template , detail::is_usable_as_key_type::value, int> = 0> size_type erase(KeyType && key) // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it != this->end()) { - if (m_compare(it->first, key)) - { - // Since we cannot move const Keys, re-construct them in place - for (auto next = it; ++next != this->end(); ++it) - { - it->~value_type(); // Destroy but keep allocation - new (&*it) value_type{std::move(*next)}; - } - Container::pop_back(); - return 1; - } + erase_at(it); + return 1; } return 0; } @@ -278,80 +275,38 @@ template , size_type count(const key_type& key) const { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return 1; - } - } - return 0; + return find_impl(*this, key) != this->end() ? 1 : 0; } template::value, int> = 0> size_type count(KeyType && key) const // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return 1; - } - } - return 0; + return find_impl(*this, key) != this->end() ? 1 : 0; } iterator find(const key_type& key) { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return it; - } - } - return Container::end(); + return find_impl(*this, key); } template::value, int> = 0> iterator find(KeyType && key) // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return it; - } - } - return Container::end(); + return find_impl(*this, key); } const_iterator find(const key_type& key) const { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return it; - } - } - return Container::end(); + return find_impl(*this, key); } template::value, int> = 0> const_iterator find(KeyType && key) const // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return it; - } - } - return Container::end(); + return find_impl(*this, key); } std::pair insert( value_type&& value ) @@ -361,12 +316,10 @@ template , std::pair insert( const value_type& value ) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, value.first); + if (it != this->end()) { - if (m_compare(it->first, value.first)) - { - return {it, false}; - } + return {it, false}; } Container::push_back(value); return {--this->end(), true}; diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 9378e6bde..f358de29a 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -25831,14 +25831,41 @@ template , return *this; } +private: + /// @brief find the entry for @a key, for either constness of @a self + /// @note the single place that performs the linear key search + template + static auto find_impl(Self& self, KeyType&& key) -> decltype(self.begin()) + { + for (auto it = self.begin(); it != self.end(); ++it) + { + if (self.m_compare(it->first, key)) + { + return it; + } + } + return self.end(); + } + + /// @brief remove the entry @a it points to, preserving order + /// @note keys are not movable, so the tail is destroyed and re-constructed in place + void erase_at(iterator it) + { + for (auto next = it; ++next != this->end(); ++it) + { + it->~value_type(); // Destroy but keep allocation + new (&*it) value_type{std::move(*next)}; + } + Container::pop_back(); + } + +public: std::pair emplace(const key_type& key, T&& t) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it != this->end()) { - if (m_compare(it->first, key)) - { - return {it, false}; - } + return {it, false}; } Container::emplace_back(key, std::forward(t)); return {std::prev(this->end()), true}; @@ -25848,12 +25875,10 @@ template , detail::is_usable_as_key_type::value, int> = 0> std::pair emplace(KeyType && key, T && t) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it != this->end()) { - if (m_compare(it->first, key)) - { - return {it, false}; - } + return {it, false}; } Container::emplace_back(std::forward(key), std::forward(t)); return {std::prev(this->end()), true}; @@ -25885,75 +25910,55 @@ template , T& at(const key_type& key) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it == this->end()) { - if (m_compare(it->first, key)) - { - return it->second; - } + JSON_THROW(std::out_of_range("key not found")); } - - JSON_THROW(std::out_of_range("key not found")); + return it->second; } template::value, int> = 0> T & at(KeyType && key) // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it == this->end()) { - if (m_compare(it->first, key)) - { - return it->second; - } + JSON_THROW(std::out_of_range("key not found")); } - - JSON_THROW(std::out_of_range("key not found")); + return it->second; } const T& at(const key_type& key) const { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it == this->end()) { - if (m_compare(it->first, key)) - { - return it->second; - } + JSON_THROW(std::out_of_range("key not found")); } - - JSON_THROW(std::out_of_range("key not found")); + return it->second; } template::value, int> = 0> const T & at(KeyType && key) const // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it == this->end()) { - if (m_compare(it->first, key)) - { - return it->second; - } + JSON_THROW(std::out_of_range("key not found")); } - - JSON_THROW(std::out_of_range("key not found")); + return it->second; } size_type erase(const key_type& key) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it != this->end()) { - if (m_compare(it->first, key)) - { - // Since we cannot move const Keys, re-construct them in place - for (auto next = it; ++next != this->end(); ++it) - { - it->~value_type(); // Destroy but keep allocation - new (&*it) value_type{std::move(*next)}; - } - Container::pop_back(); - return 1; - } + erase_at(it); + return 1; } return 0; } @@ -25962,19 +25967,11 @@ template , detail::is_usable_as_key_type::value, int> = 0> size_type erase(KeyType && key) // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, key); + if (it != this->end()) { - if (m_compare(it->first, key)) - { - // Since we cannot move const Keys, re-construct them in place - for (auto next = it; ++next != this->end(); ++it) - { - it->~value_type(); // Destroy but keep allocation - new (&*it) value_type{std::move(*next)}; - } - Container::pop_back(); - return 1; - } + erase_at(it); + return 1; } return 0; } @@ -26039,80 +26036,38 @@ template , size_type count(const key_type& key) const { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return 1; - } - } - return 0; + return find_impl(*this, key) != this->end() ? 1 : 0; } template::value, int> = 0> size_type count(KeyType && key) const // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return 1; - } - } - return 0; + return find_impl(*this, key) != this->end() ? 1 : 0; } iterator find(const key_type& key) { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return it; - } - } - return Container::end(); + return find_impl(*this, key); } template::value, int> = 0> iterator find(KeyType && key) // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return it; - } - } - return Container::end(); + return find_impl(*this, key); } const_iterator find(const key_type& key) const { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return it; - } - } - return Container::end(); + return find_impl(*this, key); } template::value, int> = 0> const_iterator find(KeyType && key) const // NOLINT(cppcoreguidelines-missing-std-forward) { - for (auto it = this->begin(); it != this->end(); ++it) - { - if (m_compare(it->first, key)) - { - return it; - } - } - return Container::end(); + return find_impl(*this, key); } std::pair insert( value_type&& value ) @@ -26122,12 +26077,10 @@ template , std::pair insert( const value_type& value ) { - for (auto it = this->begin(); it != this->end(); ++it) + const auto it = find_impl(*this, value.first); + if (it != this->end()) { - if (m_compare(it->first, value.first)) - { - return {it, false}; - } + return {it, false}; } Container::push_back(value); return {--this->end(), true};