From 700b95f4473cac1d900b807541daad0ca3d98f52 Mon Sep 17 00:00:00 2001 From: Florian Albrechtskirchinger Date: Tue, 8 Mar 2022 10:10:50 +0100 Subject: [PATCH] Make iterator operator++/--(int) equality-preserving (#3332) Commit f28fc22 introduced const qualifiers on post-(inc-/dec-)rement operators of iterators. These qualifiers prevent the use of basic_json in place of std::ranges::range, which requires the post-increment operator to be equality-preserving. These changes appear to be the result of ICC compiler suggestions, and no further explanation is discernible from the PR discussion (#858). Further testing revealed, that clang-tidy also suggests adding const to prevent "accidental mutation of a temporary object". As an alternative, this commit partially reverts f28fc22, removing all added const qualifiers from return types and adds lvalue reference qualifiers to the operator member functions instead. Unit tests ensure the operators remain equality-preserving and accidental mutation of temporaries following post-(inc-/dec-)rement is prohibited. Fixes #3331. --- .../nlohmann/detail/iterators/iter_impl.hpp | 4 +- .../iterators/json_reverse_iterator.hpp | 4 +- .../detail/iterators/primitive_iterator.hpp | 4 +- single_include/nlohmann/json.hpp | 12 +-- test/src/unit-class_iterator.cpp | 91 +++++++++++++++++++ 5 files changed, 103 insertions(+), 12 deletions(-) diff --git a/include/nlohmann/detail/iterators/iter_impl.hpp b/include/nlohmann/detail/iterators/iter_impl.hpp index 434a62d3e..d8060786e 100644 --- a/include/nlohmann/detail/iterators/iter_impl.hpp +++ b/include/nlohmann/detail/iterators/iter_impl.hpp @@ -352,7 +352,7 @@ class iter_impl // NOLINT(cppcoreguidelines-special-member-functions,hicpp-speci @brief post-increment (it++) @pre The iterator is initialized; i.e. `m_object != nullptr`. */ - iter_impl const operator++(int) // NOLINT(readability-const-return-type) + iter_impl operator++(int)& // NOLINT(cert-dcl21-cpp) { auto result = *this; ++(*this); @@ -403,7 +403,7 @@ class iter_impl // NOLINT(cppcoreguidelines-special-member-functions,hicpp-speci @brief post-decrement (it--) @pre The iterator is initialized; i.e. `m_object != nullptr`. */ - iter_impl const operator--(int) // NOLINT(readability-const-return-type) + iter_impl operator--(int)& // NOLINT(cert-dcl21-cpp) { auto result = *this; --(*this); diff --git a/include/nlohmann/detail/iterators/json_reverse_iterator.hpp b/include/nlohmann/detail/iterators/json_reverse_iterator.hpp index e787fdbcd..65bb327a5 100644 --- a/include/nlohmann/detail/iterators/json_reverse_iterator.hpp +++ b/include/nlohmann/detail/iterators/json_reverse_iterator.hpp @@ -48,7 +48,7 @@ class json_reverse_iterator : public std::reverse_iterator explicit json_reverse_iterator(const base_iterator& it) noexcept : base_iterator(it) {} /// post-increment (it++) - json_reverse_iterator const operator++(int) // NOLINT(readability-const-return-type) + json_reverse_iterator operator++(int)& // NOLINT(cert-dcl21-cpp) { return static_cast(base_iterator::operator++(1)); } @@ -60,7 +60,7 @@ class json_reverse_iterator : public std::reverse_iterator } /// post-decrement (it--) - json_reverse_iterator const operator--(int) // NOLINT(readability-const-return-type) + json_reverse_iterator operator--(int)& // NOLINT(cert-dcl21-cpp) { return static_cast(base_iterator::operator--(1)); } diff --git a/include/nlohmann/detail/iterators/primitive_iterator.hpp b/include/nlohmann/detail/iterators/primitive_iterator.hpp index 15aa2f08a..03bc37e2c 100644 --- a/include/nlohmann/detail/iterators/primitive_iterator.hpp +++ b/include/nlohmann/detail/iterators/primitive_iterator.hpp @@ -87,7 +87,7 @@ class primitive_iterator_t return *this; } - primitive_iterator_t const operator++(int) noexcept // NOLINT(readability-const-return-type) + primitive_iterator_t operator++(int)& noexcept // NOLINT(cert-dcl21-cpp) { auto result = *this; ++m_it; @@ -100,7 +100,7 @@ class primitive_iterator_t return *this; } - primitive_iterator_t const operator--(int) noexcept // NOLINT(readability-const-return-type) + primitive_iterator_t operator--(int)& noexcept // NOLINT(cert-dcl21-cpp) { auto result = *this; --m_it; diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 48999920d..05000fbba 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -11308,7 +11308,7 @@ class primitive_iterator_t return *this; } - primitive_iterator_t const operator++(int) noexcept // NOLINT(readability-const-return-type) + primitive_iterator_t operator++(int)& noexcept // NOLINT(cert-dcl21-cpp) { auto result = *this; ++m_it; @@ -11321,7 +11321,7 @@ class primitive_iterator_t return *this; } - primitive_iterator_t const operator--(int) noexcept // NOLINT(readability-const-return-type) + primitive_iterator_t operator--(int)& noexcept // NOLINT(cert-dcl21-cpp) { auto result = *this; --m_it; @@ -11728,7 +11728,7 @@ class iter_impl // NOLINT(cppcoreguidelines-special-member-functions,hicpp-speci @brief post-increment (it++) @pre The iterator is initialized; i.e. `m_object != nullptr`. */ - iter_impl const operator++(int) // NOLINT(readability-const-return-type) + iter_impl operator++(int)& // NOLINT(cert-dcl21-cpp) { auto result = *this; ++(*this); @@ -11779,7 +11779,7 @@ class iter_impl // NOLINT(cppcoreguidelines-special-member-functions,hicpp-speci @brief post-decrement (it--) @pre The iterator is initialized; i.e. `m_object != nullptr`. */ - iter_impl const operator--(int) // NOLINT(readability-const-return-type) + iter_impl operator--(int)& // NOLINT(cert-dcl21-cpp) { auto result = *this; --(*this); @@ -12167,7 +12167,7 @@ class json_reverse_iterator : public std::reverse_iterator explicit json_reverse_iterator(const base_iterator& it) noexcept : base_iterator(it) {} /// post-increment (it++) - json_reverse_iterator const operator++(int) // NOLINT(readability-const-return-type) + json_reverse_iterator operator++(int)& // NOLINT(cert-dcl21-cpp) { return static_cast(base_iterator::operator++(1)); } @@ -12179,7 +12179,7 @@ class json_reverse_iterator : public std::reverse_iterator } /// post-decrement (it--) - json_reverse_iterator const operator--(int) // NOLINT(readability-const-return-type) + json_reverse_iterator operator--(int)& // NOLINT(cert-dcl21-cpp) { return static_cast(base_iterator::operator--(1)); } diff --git a/test/src/unit-class_iterator.cpp b/test/src/unit-class_iterator.cpp index 0e159fc38..e3a972f97 100644 --- a/test/src/unit-class_iterator.cpp +++ b/test/src/unit-class_iterator.cpp @@ -33,6 +33,12 @@ SOFTWARE. #include using nlohmann::json; +template +using can_post_increment_temporary = decltype((std::declval()++)++); + +template +using can_post_decrement_temporary = decltype((std::declval()--)--); + TEST_CASE("iterator class") { SECTION("construction") @@ -399,4 +405,89 @@ TEST_CASE("iterator class") } } } + SECTION("equality-preserving") + { + SECTION("post-increment") + { + SECTION("primitive_iterator_t") + { + using Iter = nlohmann::detail::primitive_iterator_t; + CHECK(std::is_same < decltype(std::declval()++), Iter >::value); + } + SECTION("iter_impl") + { + using Iter = nlohmann::detail::iter_impl; + CHECK(std::is_same < decltype(std::declval()++), Iter >::value); + } + SECTION("json_reverse_iterator") + { + using Base = nlohmann::detail::iter_impl; + using Iter = nlohmann::detail::json_reverse_iterator; + CHECK(std::is_same < decltype(std::declval()++), Iter >::value); + } + } + SECTION("post-decrement") + { + SECTION("primitive_iterator_t") + { + using Iter = nlohmann::detail::primitive_iterator_t; + CHECK(std::is_same < decltype(std::declval()--), Iter >::value); + } + SECTION("iter_impl") + { + using Iter = nlohmann::detail::iter_impl; + CHECK(std::is_same < decltype(std::declval()--), Iter >::value ); + } + SECTION("json_reverse_iterator") + { + using Base = nlohmann::detail::iter_impl; + using Iter = nlohmann::detail::json_reverse_iterator; + CHECK(std::is_same < decltype(std::declval()--), Iter >::value ); + } + } + } + // prevent "accidental mutation of a temporary object" + SECTION("cert-dcl21-cpp") + { + using nlohmann::detail::is_detected; + SECTION("post-increment") + { + SECTION("primitive_iterator_t") + { + using Iter = nlohmann::detail::primitive_iterator_t; + CHECK_FALSE(is_detected::value); + } + SECTION("iter_impl") + { + using Iter = nlohmann::detail::iter_impl; + CHECK_FALSE(is_detected::value); + } + SECTION("json_reverse_iterator") + { + using Base = nlohmann::detail::iter_impl; + using Iter = nlohmann::detail::json_reverse_iterator; + CHECK_FALSE(is_detected::value); + } + } + SECTION("post-decrement") + { + SECTION("primitive_iterator_t") + { + using Iter = nlohmann::detail::primitive_iterator_t; + CHECK_FALSE(is_detected::value); + } + SECTION("iter_impl") + { + using Iter = nlohmann::detail::iter_impl; + CHECK_FALSE(is_detected::value); + } + SECTION("json_reverse_iterator") + { + using Base = nlohmann::detail::iter_impl; + using Iter = nlohmann::detail::json_reverse_iterator; + CHECK_FALSE(is_detected::value); + } + + } + } }