Deduplicate basic_json internals; make insert(pos, json&&) move

.clang-tidy

@@ -1,18 +1,9 @@
-# bugprone-use-after-move (hicpp-invalid-access-moved is its alias) still flags
-# the basic_json move constructor, which forwards the whole object to its base
-# class (#5724), and two forwards in the error-message construction of
-# at(KeyType&&) (json.hpp, both overloads: find(std::forward<KeyType>(key))
-# followed by string_t(std::forward<KeyType>(key)) in the throw), which #5689
-# rewrites. Re-enable both checks once those changes have landed.
 # portability-avoid-pragma-once: kept disabled on purpose. #pragma once is accepted
 # by every supported compiler, and tools/amalgamate/amalgamate.py strips it from
 # single_include, so there is nothing left to fix here.
 
 Checks: '*,
 
-         -bugprone-use-after-move,
-         -hicpp-invalid-access-moved,
-
          -altera-id-dependent-backward-branch,
          -altera-struct-pack-align,
          -altera-unroll-loops,

docs/mkdocs/docs/api/basic_json/meta.md

@@ -13,7 +13,7 @@ JSON object holding version information
 
 | key         | description                                                                                                                                                                                                                                                                    |
 |-------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
-| `compiler`  | Information on the used compiler. It is an object with the following keys: `c++` (the used C++ standard), `family` (the compiler family; possible values are `clang`, `icc`, `gcc`, `ilecpp`, `msvc`, `pgcpp`, `sunpro`, and `unknown`), and `version` (the compiler version). On HP aCC compilers, `compiler` is instead the plain string `hp`. |
+| `compiler`  | Information on the used compiler. It is an object with the following keys: `c++` (the used C++ standard), `family` (the compiler family; possible values are `clang`, `icc`, `gcc`, `hp`, `ilecpp`, `msvc`, `pgcpp`, `sunpro`, and `unknown`), and `version` (the compiler version). |
 | `copyright` | The copyright line for the library as string.                                                                                                                                                                                                                                  |
 | `name`      | The name of the library as string.                                                                                                                                                                                                                                             |
 | `platform`  | The used platform as string. Possible values are `win32`, `linux`, `apple`, `unix`, and `unknown`.                                                                                                                                                                             |

include/nlohmann/detail/input/input_adapters.hpp

@@ -746,7 +746,7 @@ struct container_input_adapter_factory< ContainerType,
 {
     // container is forwarded twice on purpose: the resulting begin/end
     // iterator types must match adapter_type, computed the same way
-    // NOLINTNEXTLINE(bugprone-use-after-move)
+    // NOLINTNEXTLINE(bugprone-use-after-move,hicpp-invalid-access-moved)
     return input_adapter(begin(std::forward<ContainerType>(container)), end(std::forward<ContainerType>(container)));
 }
        };

include/nlohmann/detail/string_concat.hpp

@@ -39,7 +39,6 @@ inline std::size_t concat_length(const char /*c*/, const Args& ... rest)
 template<typename... Args>
 inline std::size_t concat_length(const char* cstr, const Args& ... rest)
 {
-    // cppcheck-suppress ignoredReturnValue
     return ::strlen(cstr) + concat_length(rest...);
 }
 

include/nlohmann/ordered_map.hpp

@@ -74,16 +74,43 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
         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<typename Self, typename KeyType>
+    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:
     template<class V, detail::enable_if_t<
                  detail::is_constructible<T, V>::value, int> = 0>
     std::pair<iterator, bool> emplace(const key_type& key, V && 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};
         }
         append(key, std::forward<V>(t));
         return {std::prev(this->end()), true};
@@ -94,12 +121,10 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
                                      detail::is_constructible<T, V>>::value, int> = 0>
     std::pair<iterator, bool> emplace(KeyType && key, V && 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};
         }
         append(std::forward<KeyType>(key), std::forward<V>(t));
         return {std::prev(this->end()), true};
@@ -131,75 +156,55 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
 
     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<class KeyType, detail::enable_if_t<
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::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<class KeyType, detail::enable_if_t<
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::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;
     }
@@ -208,19 +213,11 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::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;
     }
@@ -285,80 +282,38 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
 
     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<class KeyType, detail::enable_if_t<
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::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<class KeyType, detail::enable_if_t<
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::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<class KeyType, detail::enable_if_t<
                  detail::is_usable_as_key_type<key_compare, key_type, KeyType>::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<iterator, bool> insert( value_type&& value )
@@ -368,12 +323,10 @@ template <class Key, class T, class IgnoredLess = std::less<Key>,
 
     std::pair<iterator, bool> 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};
         }
         append(value);
         return {--this->end(), true};

tests/src/unit-class_parser.cpp

@@ -2684,12 +2684,10 @@ TEST_CASE("diagnostic positions: value lifetime, input adapters, and SAX")
 
         SECTION("move constructor resets the moved-from value to npos")
         {
-            // basic_json(basic_json&&) (json.hpp, around line 1951) copies
+            // basic_json(basic_json&&) copies
             // other's start_position/end_position into *this and then resets
-            // other's to npos (see the cppcheck-suppress[accessForwarded]
-            // annotation there, which flags this reset as worth a second
-            // look). Only the top-level moved-from value is affected; its
-            // (moved-away) children are gone along with it.
+            // other's to npos. Only the top-level moved-from value is
+            // affected; its (moved-away) children are gone along with it.
             const std::string s = R"({"a":1,"b":[1,2,3]})";
             json a = json::parse(s);
             const auto a_start = a.start_pos();

tests/src/unit-modifiers.cpp

@@ -630,6 +630,49 @@ TEST_CASE("modifiers")
             }
         }
 
+        SECTION("rvalue at position moves rather than copies")
+        {
+            // regression test: insert(pos, basic_json&&) used to forward to
+            // insert(pos, const basic_json&) because the named rvalue
+            // reference parameter is itself an lvalue, so it always
+            // deep-copied its argument instead of moving it
+            json j_big = std::string(1000, 'x');
+            const auto* const original_buffer = j_big.get_ref<const std::string&>().data();
+
+            auto it = j_array.insert(j_array.begin(), std::move(j_big));
+            CHECK(j_array.size() == 5);
+            CHECK(*it == json(std::string(1000, 'x')));
+            CHECK((*it).get_ref<const std::string&>().data() == original_buffer);
+
+            // the moved-from value is null, the same as after push_back(&&)
+            CHECK(j_big.is_null()); // NOLINT(bugprone-use-after-move,hicpp-invalid-access-moved)
+        }
+
+        SECTION("self-aliasing insertion")
+        {
+            SECTION("without reallocation")
+            {
+                json j_self = {1, 2, 3, 4};
+                j_self.get_ref<json::array_t&>().reserve(j_self.size() + 1);
+
+                auto it = j_self.insert(j_self.begin(), std::move(j_self[1]));
+                CHECK(j_self.size() == 5);
+                CHECK(*it == json(2));
+                CHECK(j_self == json({2, 1, nullptr, 3, 4}));
+            }
+
+            SECTION("with reallocation")
+            {
+                json j_self = {1, 2, 3, 4};
+                j_self.get_ref<json::array_t&>().shrink_to_fit();
+
+                auto it = j_self.insert(j_self.begin(), std::move(j_self[1]));
+                CHECK(j_self.size() == 5);
+                CHECK(*it == json(2));
+                CHECK(j_self == json({2, 1, nullptr, 3, 4}));
+            }
+        }
+
         SECTION("copies at position")
         {
             SECTION("insert before begin()")