Address review findings on the binary writer output sinks

- binary_reserve_hint(): the 4-bytes-per-element estimate over-reserved by up
  to 4x for arrays of small scalars (CBOR encodes 0..23 in one byte), and the
  returned vector kept that capacity. Make the hint a strict lower bound on the
  encoded size instead, which also removes the 1 MiB clamp whose branch no test
  could reach (the largest container in the suite has 65793 elements).

- Guard the -Wduplicated-branches pragma with __GNUC__ >= 7. The warning does
  not exist before GCC 7, so naming it made GCC 4.8/4.9/5/6 - which the CI
  matrix still builds - warn under -Wpragmas on every including translation
  unit, breaking downstream -Werror builds.

- Constrain the adapter constructor of binary_writer with the enable_if its
  documentation already claimed, so a writer over some other sink type is no
  longer advertised as constructible from an output adapter.

- Let output_vector_adapter wrap output_vector_sink rather than duplicating the
  append logic, so the type-erased and templated paths share one implementation.

- Collapse the three copies of the memcpy/byte_swap/memcpy dance into a single
  byte_swap_buffer() helper, and add the MSVC _byteswap_* intrinsics so MSVC no
  longer falls back to the scalar shuffle this change exists to eliminate.

- Add a vector_writer() helper for the five vector-returning to_* overloads
  instead of spelling out the writer type at each call site, and drop a dead
  default member initializer on output_adapter_sink.

- New tests: the vector sink and the adapter sink must produce identical bytes
  for every format (the two to_* overloads no longer delegate to each other and
  could otherwise drift), and binary_reserve_hint() must never exceed the size
  actually written.

Signed-off-by: Niels Lohmann <mail@nlohmann.me>

include/nlohmann/detail/output/binary_writer.hpp

@@ -16,9 +16,14 @@
 #include <cstring> // memcpy
 #include <limits> // numeric_limits
 #include <string> // string
+#include <type_traits> // enable_if, is_constructible
 #include <utility> // move
 #include <vector> // vector
 
+#ifdef _MSC_VER
+    #include <cstdlib> // _byteswap_ushort, _byteswap_ulong, _byteswap_uint64
+#endif
+
 #include <nlohmann/detail/input/binary_reader.hpp>
 #include <nlohmann/detail/macro_scope.hpp>
 #include <nlohmann/detail/output/output_adapters.hpp>
@@ -40,31 +45,33 @@ enum class bjdata_version_t
 ///////////////////
 
 /*!
-@brief conservative capacity hint for binary serialization into a std::vector
+@brief capacity hint for binary serialization into a std::vector
 
-Returns an approximate number of bytes to reserve up front so that serializing
-an array/object of many elements does not repeatedly reallocate the output
-buffer. Only the top-level element count is consulted (O(1), no walk of the
-DOM), and the result is clamped to a fixed ceiling: a large or untrusted DOM can
-therefore never trigger an oversized allocation here, and the multiplication
-cannot overflow. The buffer still grows geometrically beyond the hint, so a hint
-that is too small only costs a few later reallocations. A single scalar, string,
-or binary value is written in one shot and needs no hint.
+Returns a *lower* bound on the number of bytes the serialization will produce,
+so that writing an array/object of many elements does not start reallocating
+from an empty buffer. Every array element occupies at least one byte in every
+supported binary format, and every object entry at least two (a key of at least
+one byte plus a value of at least one), plus one byte for the container header,
+so the hint can never exceed the final size and the returned vector is never
+left holding capacity the caller did not ask for. The buffer still grows
+geometrically past the hint, so under-reserving only costs a few later
+reallocations. Only the top-level element count is consulted (O(1), no walk of
+the DOM); a single scalar, string, or binary value is written in one shot and
+needs no hint.
 */
 template<typename BasicJsonType>
 std::size_t binary_reserve_hint(const BasicJsonType& j)
 {
-    constexpr std::size_t max_hint = static_cast<std::size_t>(1) << 20; // 1 MiB
-    if (j.is_array() || j.is_object())
+    if (j.is_array())
     {
-        const std::size_t elements = j.size();
-        // guard the multiplication against overflow and cap the reservation
-        if (elements > max_hint / 4)
-        {
-            return max_hint;
-        }
-        return (elements * 4) + 2;
+        return j.size() + 1;
     }
+
+    if (j.is_object())
+    {
+        return (j.size() * 2) + 1;
+    }
+
     return 0;
 }
 
@@ -94,12 +101,15 @@ class binary_writer
 
     Convenience constructor for the default (output_adapter_sink) sink so the
     `output_adapter`-based overloads keep constructing the writer directly from
-    an adapter. Only participates in overload resolution when the sink can be
-    built from an adapter.
+    an adapter. Constrained to sinks that can actually be built from an adapter,
+    so that a writer over some other sink type is not advertised as constructible
+    from one.
 
     @param[in] adapter  output adapter to write to
     */
-    explicit binary_writer(output_adapter_t<CharType> adapter) : oa(OutputSinkType(std::move(adapter)))
+    template < typename SinkType = OutputSinkType,
+               typename std::enable_if < std::is_constructible<SinkType, output_adapter_t<CharType>>::value, int >::type = 0 >
+    explicit binary_writer(output_adapter_t<CharType> adapter) : oa(SinkType(std::move(adapter)))
     {}
 
     /*!
@@ -1860,6 +1870,8 @@ class binary_writer
     {
 #if defined(__GNUC__) || defined(__clang__)
         return __builtin_bswap16(x);
+#elif defined(_MSC_VER)
+        return _byteswap_ushort(x);
 #else
         return static_cast<std::uint16_t>((x >> 8) | (x << 8));
 #endif
@@ -1869,6 +1881,8 @@ class binary_writer
     {
 #if defined(__GNUC__) || defined(__clang__)
         return __builtin_bswap32(x);
+#elif defined(_MSC_VER)
+        return _byteswap_ulong(x);
 #else
         return ((x & 0x000000FFu) << 24) | ((x & 0x0000FF00u) << 8)
                | ((x & 0x00FF0000u) >> 8) | ((x & 0xFF000000u) >> 24);
@@ -1879,6 +1893,8 @@ class binary_writer
     {
 #if defined(__GNUC__) || defined(__clang__)
         return __builtin_bswap64(x);
+#elif defined(_MSC_VER)
+        return _byteswap_uint64(x);
 #else
         x = ((x & 0x00000000FFFFFFFFull) << 32) | ((x & 0xFFFFFFFF00000000ull) >> 32);
         x = ((x & 0x0000FFFF0000FFFFull) << 16) | ((x & 0xFFFF0000FFFF0000ull) >> 16);
@@ -1887,30 +1903,40 @@ class binary_writer
 #endif
     }
 
+    /*!
+    @brief reverse the bytes of a buffer by byte-swapping it as UIntType
+
+    Loading the buffer into an unsigned integer of the same width and swapping
+    that is what lets the compiler emit a single bswap/rev/movbe; reversing the
+    buffer element by element does not reliably get there (clang keeps a scalar
+    shuffle). The two memcpy calls are the only portable way to reinterpret the
+    bytes and are folded away by every optimizer.
+    */
+    template<typename UIntType, std::size_t N>
+    static void byte_swap_buffer(std::array<CharType, N>& a) noexcept
+    {
+        static_assert(sizeof(UIntType) == N, "swap width must match the buffer size");
+        UIntType v{};
+        std::memcpy(&v, a.data(), sizeof(v));
+        v = byte_swap(v);
+        std::memcpy(a.data(), &v, sizeof(v));
+    }
+
     // reverse the bytes of a fixed-size buffer; a single byte_swap() for the
     // common 2/4/8-byte number payloads, std::reverse for any other size
     static void reverse_bytes(std::array<CharType, 2>& a) noexcept
     {
-        std::uint16_t v{};
-        std::memcpy(&v, a.data(), sizeof(v));
-        v = byte_swap(v);
-        std::memcpy(a.data(), &v, sizeof(v));
+        byte_swap_buffer<std::uint16_t>(a);
     }
 
     static void reverse_bytes(std::array<CharType, 4>& a) noexcept
     {
-        std::uint32_t v{};
-        std::memcpy(&v, a.data(), sizeof(v));
-        v = byte_swap(v);
-        std::memcpy(a.data(), &v, sizeof(v));
+        byte_swap_buffer<std::uint32_t>(a);
     }
 
     static void reverse_bytes(std::array<CharType, 8>& a) noexcept
     {
-        std::uint64_t v{};
-        std::memcpy(&v, a.data(), sizeof(v));
-        v = byte_swap(v);
-        std::memcpy(a.data(), &v, sizeof(v));
+        byte_swap_buffer<std::uint64_t>(a);
     }
 
     template<std::size_t N>
@@ -1946,7 +1972,9 @@ class binary_writer
         // both branches below are intentionally identical (the "compact" float
         // representation is the value itself). Only GCC diagnoses this, and only
         // when the sink calls are inlined; clang has no such warning.
-#if defined(__GNUC__) && !defined(__clang__)
+        // (-Wduplicated-branches only exists from GCC 7 on; naming it on an older
+        // GCC would itself warn under -Wpragmas)
+#if defined(__GNUC__) && !defined(__clang__) && (__GNUC__ >= 7)
 #pragma GCC diagnostic ignored "-Wduplicated-branches"
 #endif
         if (!std::isfinite(n) || ((static_cast<double>(n) >= static_cast<double>(std::numeric_limits<float>::lowest()) &&

include/nlohmann/detail/output/output_adapters.hpp

@@ -45,22 +45,32 @@ template<typename CharType> struct output_adapter_protocol
 template<typename CharType>
 using output_adapter_t = std::shared_ptr<output_adapter_protocol<CharType>>;
 
-/// output adapter for byte vectors
+/// @brief non-virtual output sink writing into a std::vector
+///
+/// This sink is not part of the virtual output_adapter_protocol hierarchy: it is
+/// passed to binary_writer by value as a template parameter, so
+/// write_character()/write_characters() are ordinary (inlinable) calls with no
+/// vtable lookup and no shared_ptr. It is used for the common
+/// `to_cbor`/`to_msgpack`/... into a std::vector. output_vector_adapter below
+/// wraps this same sink to provide the virtual interface.
 template<typename CharType, typename AllocatorType = std::allocator<CharType>>
-class output_vector_adapter : public output_adapter_protocol<CharType>
+class output_vector_sink
 {
   public:
-    explicit output_vector_adapter(std::vector<CharType, AllocatorType>& vec) noexcept
+    explicit output_vector_sink(std::vector<CharType, AllocatorType>& vec) noexcept
         : v(vec)
     {}
 
-    void write_character(CharType c) override
+    void write_character(CharType c)
     {
         v.push_back(c);
     }
 
-    JSON_HEDLEY_NON_NULL(2)
-    void write_characters(const CharType* s, std::size_t length) override
+    // no JSON_HEDLEY_NON_NULL here: binary_writer legitimately passes a null
+    // pointer with length 0 for empty strings/binary values. Appending an empty
+    // range is a no-op; the type-erased path tolerates this via the (unattributed)
+    // virtual base, and the concrete sink must do the same.
+    void write_characters(const CharType* s, std::size_t length)
     {
         v.insert(v.end(), s, s + length);
     }
@@ -69,6 +79,34 @@ class output_vector_adapter : public output_adapter_protocol<CharType>
     std::vector<CharType, AllocatorType>& v;
 };
 
+/// output adapter for byte vectors
+///
+/// The appending itself lives in output_vector_sink; this class only adds the
+/// virtual output_adapter_protocol interface on top of it, so both the
+/// type-erased and the templated path share one implementation.
+template<typename CharType, typename AllocatorType = std::allocator<CharType>>
+class output_vector_adapter : public output_adapter_protocol<CharType>
+{
+  public:
+    explicit output_vector_adapter(std::vector<CharType, AllocatorType>& vec) noexcept
+        : sink(vec)
+    {}
+
+    void write_character(CharType c) override
+    {
+        sink.write_character(c);
+    }
+
+    JSON_HEDLEY_NON_NULL(2)
+    void write_characters(const CharType* s, std::size_t length) override
+    {
+        sink.write_characters(s, length);
+    }
+
+  private:
+    output_vector_sink<CharType, AllocatorType> sink;
+};
+
 #ifndef JSON_NO_IO
 /// output adapter for output streams
 template<typename CharType>
@@ -119,39 +157,6 @@ class output_string_adapter : public output_adapter_protocol<CharType>
     StringType& str;
 };
 
-/// @brief non-virtual output sink writing into a std::vector
-///
-/// Unlike output_vector_adapter, this sink is not part of the virtual
-/// output_adapter_protocol hierarchy: it is passed to binary_writer by value as
-/// a template parameter, so write_character()/write_characters() are ordinary
-/// (inlinable) calls with no vtable lookup and no shared_ptr. It is used for the
-/// common `to_cbor`/`to_msgpack`/... into a std::vector.
-template<typename CharType, typename AllocatorType = std::allocator<CharType>>
-class output_vector_sink
-{
-  public:
-    explicit output_vector_sink(std::vector<CharType, AllocatorType>& vec) noexcept
-        : v(vec)
-    {}
-
-    void write_character(CharType c)
-    {
-        v.push_back(c);
-    }
-
-    // no JSON_HEDLEY_NON_NULL here: binary_writer legitimately passes a null
-    // pointer with length 0 for empty strings/binary values. Appending an empty
-    // range is a no-op; the type-erased path tolerates this via the (unattributed)
-    // virtual base, and the concrete sink must do the same.
-    void write_characters(const CharType* s, std::size_t length)
-    {
-        v.insert(v.end(), s, s + length);
-    }
-
-  private:
-    std::vector<CharType, AllocatorType>& v;
-};
-
 /// @brief output sink forwarding to a type-erased output adapter
 ///
 /// Wraps the polymorphic output_adapter_t so the same binary_writer template can
@@ -182,7 +187,7 @@ class output_adapter_sink
     }
 
   private:
-    output_adapter_t<CharType> oa = nullptr;
+    output_adapter_t<CharType> oa;
 };
 
 template<typename CharType, typename StringType = std::basic_string<CharType>>

include/nlohmann/json.hpp

@@ -187,6 +187,14 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     template<typename InputType>
     using binary_reader = ::nlohmann::detail::binary_reader<basic_json, InputType>;
     template<typename CharType> using binary_writer = ::nlohmann::detail::binary_writer<basic_json, CharType>;
+    // binary_writer over a concrete (non-virtual) sink appending into a std::vector,
+    // used by the vector-returning to_* overloads
+    template<typename CharType> using vector_binary_writer =
+        ::nlohmann::detail::binary_writer<basic_json, CharType, ::nlohmann::detail::output_vector_sink<CharType>>;
+    template<typename CharType> static vector_binary_writer<CharType> vector_writer(std::vector<CharType>& v)
+    {
+        return vector_binary_writer<CharType>(::nlohmann::detail::output_vector_sink<CharType>(v));
+    }
 
   JSON_PRIVATE_UNLESS_TESTED:
     using serializer = ::nlohmann::detail::serializer<basic_json>;
@@ -4338,8 +4346,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     {
         std::vector<std::uint8_t> result;
         result.reserve(detail::binary_reserve_hint(j));
-        detail::binary_writer<basic_json, std::uint8_t, detail::output_vector_sink<std::uint8_t>>(
-            detail::output_vector_sink<std::uint8_t>(result)).write_cbor(j);
+        vector_writer(result).write_cbor(j);
         return result;
     }
 
@@ -4363,8 +4370,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     {
         std::vector<std::uint8_t> result;
         result.reserve(detail::binary_reserve_hint(j));
-        detail::binary_writer<basic_json, std::uint8_t, detail::output_vector_sink<std::uint8_t>>(
-            detail::output_vector_sink<std::uint8_t>(result)).write_msgpack(j);
+        vector_writer(result).write_msgpack(j);
         return result;
     }
 
@@ -4390,8 +4396,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     {
         std::vector<std::uint8_t> result;
         result.reserve(detail::binary_reserve_hint(j));
-        detail::binary_writer<basic_json, std::uint8_t, detail::output_vector_sink<std::uint8_t>>(
-            detail::output_vector_sink<std::uint8_t>(result)).write_ubjson(j, use_size, use_type);
+        vector_writer(result).write_ubjson(j, use_size, use_type);
         return result;
     }
 
@@ -4420,8 +4425,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     {
         std::vector<std::uint8_t> result;
         result.reserve(detail::binary_reserve_hint(j));
-        detail::binary_writer<basic_json, std::uint8_t, detail::output_vector_sink<std::uint8_t>>(
-            detail::output_vector_sink<std::uint8_t>(result)).write_ubjson(j, use_size, use_type, true, true, version);
+        vector_writer(result).write_ubjson(j, use_size, use_type, true, true, version);
         return result;
     }
 
@@ -4449,8 +4453,7 @@ class basic_json // NOLINT(cppcoreguidelines-special-member-functions,hicpp-spec
     {
         std::vector<std::uint8_t> result;
         result.reserve(detail::binary_reserve_hint(j));
-        detail::binary_writer<basic_json, std::uint8_t, detail::output_vector_sink<std::uint8_t>>(
-            detail::output_vector_sink<std::uint8_t>(result)).write_bson(j);
+        vector_writer(result).write_bson(j);
         return result;
     }