From b640bf728df1543d8e12743688abb95f7ede9d00 Mon Sep 17 00:00:00 2001 From: Niels Lohmann Date: Wed, 30 Sep 2026 10:05:33 +0200 Subject: [PATCH] Re-enable portability-template-virtual-member-function; remove redundant forwards .clang-tidy disabled three checks "to get the CI going" (#4489, 2024-11-13): portability-template-virtual-member-function, bugprone-use-after-move and its alias hicpp-invalid-access-moved. portability-template-virtual-member-function only flagged output_stream_adapter::write_character/write_characters; annotate both with NOLINT and re-enable the check. bugprone-use-after-move flagged several double forwards that have no effect at runtime: - from_json.hpp calls std::forward(j).at(Idx) inside pack expansions; at() has no ref-qualified overloads and always returns an lvalue reference, so the forward is a no-op. Replace with plain j.at(Idx) in all four places. - the move constructor forwards the whole object to its base class and then reads other's members. That is item 9 of #5724 (together with its cppcheck suppressions) and is left to that change. - input_adapters.hpp forwards the container twice on purpose, so the begin/end iterator types match adapter_type; annotate with NOLINT and a comment instead of changing behavior. The check still flags the move constructor (see above) and two sites in at(KeyType&&) (both overloads, json.hpp, in the throw's string_t(std::forward(key)) after find(std::forward(key))). Open PR #5689 rewrites that hunk, so bugprone-use-after-move (and hicpp-invalid-access-moved) stay disabled for now, with a comment explaining why; re-enable them once #5689 and the #5724 move-constructor change have landed. Also resolve the portability-avoid-pragma-once TODO: single_include never has #pragma once (amalgamate.py strips it) and every supported compiler accepts it in include/, so keep it disabled with an explanatory comment instead of a TODO. Fix the stale "json.hpp, around line 1265" comment in unit-class_parser.cpp, which now points at the move constructor's actual line. Behavior, the public API and the ABI do not change. Verified with clang-tidy 22.1.8 that portability-template-virtual-member-function now reports nothing, that bugprone-use-after-move/ hicpp-invalid-access-moved report only the known at(KeyType&&) and move-constructor sites, and that unit-custom-base-class, unit-constructor1, unit-conversions, unit-element_access2, unit-class_parser and unit-diagnostic-positions (JSON_DIAGNOSTIC_POSITIONS=1) compile under ASan/UBSan and pass with the same assertion counts as before. Ran make amalgamate. Part of #5725 Signed-off-by: Niels Lohmann --- .clang-tidy | 12 +++++++++--- include/nlohmann/detail/conversions/from_json.hpp | 8 ++++---- include/nlohmann/detail/input/input_adapters.hpp | 3 +++ include/nlohmann/detail/output/output_adapters.hpp | 2 ++ single_include/nlohmann/json.hpp | 13 +++++++++---- tests/src/unit-class_parser.cpp | 2 +- 6 files changed, 28 insertions(+), 12 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 29b3e9302..752f5153a 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -1,9 +1,15 @@ -# TODO: The first three checks are only removed to get the CI going. They have to be addressed at some point. -# TODO: portability-avoid-pragma-once: should be fixed eventually +# 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(key)) +# followed by string_t(std::forward(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: '*, - -portability-template-virtual-member-function, -bugprone-use-after-move, -hicpp-invalid-access-moved, diff --git a/include/nlohmann/detail/conversions/from_json.hpp b/include/nlohmann/detail/conversions/from_json.hpp index 11e40f5f4..fdbd9df18 100644 --- a/include/nlohmann/detail/conversions/from_json.hpp +++ b/include/nlohmann/detail/conversions/from_json.hpp @@ -353,7 +353,7 @@ template < typename BasicJsonType, typename T, std::size_t... Idx > std::array from_json_inplace_array_impl(BasicJsonType&& j, identity_tag> /*unused*/, index_sequence /*unused*/) { - return { { std::forward(j).at(Idx).template get()... } }; + return { { j.at(Idx).template get()... } }; } template < typename BasicJsonType, typename T, std::size_t N > @@ -502,7 +502,7 @@ using tuple_type = std::tuple < decltype(from_json_tuple_get_impl(std::declval tuple_type from_json_tuple_impl_base(BasicJsonType&& j, index_sequence /*unused*/) { - return tuple_type(from_json_tuple_get_impl(std::forward(j).at(Idx), detail::identity_tag {}, detail::priority_tag {})...); + return tuple_type(from_json_tuple_get_impl(j.at(Idx), detail::identity_tag {}, detail::priority_tag {})...); } template @@ -514,8 +514,8 @@ std::tuple<> from_json_tuple_impl_base(BasicJsonType& /*unused*/, index_sequence template < typename BasicJsonType, class A1, class A2 > std::pair from_json_tuple_impl(BasicJsonType&& j, identity_tag> /*unused*/, priority_tag<0> /*unused*/) { - return {std::forward(j).at(0).template get(), - std::forward(j).at(1).template get()}; + return {j.at(0).template get(), + j.at(1).template get()}; } template diff --git a/include/nlohmann/detail/input/input_adapters.hpp b/include/nlohmann/detail/input/input_adapters.hpp index e174775c5..fab192ced 100644 --- a/include/nlohmann/detail/input/input_adapters.hpp +++ b/include/nlohmann/detail/input/input_adapters.hpp @@ -763,6 +763,9 @@ struct container_input_adapter_factory< ContainerType, static adapter_type create(ContainerType&& container) { + // 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) return input_adapter(begin(std::forward(container)), end(std::forward(container))); } }; diff --git a/include/nlohmann/detail/output/output_adapters.hpp b/include/nlohmann/detail/output/output_adapters.hpp index 7eb73121c..679feca1c 100644 --- a/include/nlohmann/detail/output/output_adapters.hpp +++ b/include/nlohmann/detail/output/output_adapters.hpp @@ -117,12 +117,14 @@ class output_stream_adapter : public output_adapter_protocol : stream(s) {} + // NOLINTNEXTLINE(portability-template-virtual-member-function) void write_character(CharType c) override { stream.put(c); } JSON_HEDLEY_NON_NULL(2) + // NOLINTNEXTLINE(portability-template-virtual-member-function) void write_characters(const CharType* s, std::size_t length) override { stream.write(s, static_cast(length)); diff --git a/single_include/nlohmann/json.hpp b/single_include/nlohmann/json.hpp index 09955952e..c3b3dc88d 100644 --- a/single_include/nlohmann/json.hpp +++ b/single_include/nlohmann/json.hpp @@ -5850,7 +5850,7 @@ template < typename BasicJsonType, typename T, std::size_t... Idx > std::array from_json_inplace_array_impl(BasicJsonType&& j, identity_tag> /*unused*/, index_sequence /*unused*/) { - return { { std::forward(j).at(Idx).template get()... } }; + return { { j.at(Idx).template get()... } }; } template < typename BasicJsonType, typename T, std::size_t N > @@ -5999,7 +5999,7 @@ using tuple_type = std::tuple < decltype(from_json_tuple_get_impl(std::declval tuple_type from_json_tuple_impl_base(BasicJsonType&& j, index_sequence /*unused*/) { - return tuple_type(from_json_tuple_get_impl(std::forward(j).at(Idx), detail::identity_tag {}, detail::priority_tag {})...); + return tuple_type(from_json_tuple_get_impl(j.at(Idx), detail::identity_tag {}, detail::priority_tag {})...); } template @@ -6011,8 +6011,8 @@ std::tuple<> from_json_tuple_impl_base(BasicJsonType& /*unused*/, index_sequence template < typename BasicJsonType, class A1, class A2 > std::pair from_json_tuple_impl(BasicJsonType&& j, identity_tag> /*unused*/, priority_tag<0> /*unused*/) { - return {std::forward(j).at(0).template get(), - std::forward(j).at(1).template get()}; + return {j.at(0).template get(), + j.at(1).template get()}; } template @@ -8307,6 +8307,9 @@ struct container_input_adapter_factory< ContainerType, static adapter_type create(ContainerType&& container) { + // 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) return input_adapter(begin(std::forward(container)), end(std::forward(container))); } }; @@ -20256,12 +20259,14 @@ class output_stream_adapter : public output_adapter_protocol : stream(s) {} + // NOLINTNEXTLINE(portability-template-virtual-member-function) void write_character(CharType c) override { stream.put(c); } JSON_HEDLEY_NON_NULL(2) + // NOLINTNEXTLINE(portability-template-virtual-member-function) void write_characters(const CharType* s, std::size_t length) override { stream.write(s, static_cast(length)); diff --git a/tests/src/unit-class_parser.cpp b/tests/src/unit-class_parser.cpp index 3d825162e..12f7a06a5 100644 --- a/tests/src/unit-class_parser.cpp +++ b/tests/src/unit-class_parser.cpp @@ -2525,7 +2525,7 @@ 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 1265) copies + // basic_json(basic_json&&) (json.hpp, around line 1951) 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