feat: add tr_variant::visit() (#7923)

* refactor: make tr_variant work with std::visit()

* refactor: use tr_variant::visit() in tr_variant_serde

* refactor: use tr_variant::visit() in tr_variant::merge()

* refactor: simplify JsonWriter

* fix: clang-tidy misc-use-internal-linkage

* test: simplify VariantTest.visitsNodesDepthFirst()

* test: simplify VariantTest.visitNestedJsonSummarizesStructure()

* feat: add tr_variant::clone()

* test: simplify variant tests

* docs: add code comments for tr_variant::merge() and tr_variant::clone()

* fix: add stack-smashing handling for JSON parsing

* fix: clang-tidy modernize-raw-string-literal

* Use writer.Key() for object key names

* refactor: remove unnecessary FMT_COMPILE() macros

* refactor: fix tr_variant::VisitAdapter to preserve the visitor category

chore: remove unnecessary std::cref() calls when calling tr_variant::visitor()

---------

Co-authored-by: Yat Ho <lagoho7@gmail.com>

libtransmission/variant-benc.cc

@@ -13,6 +13,7 @@
 #include <string>
 #include <string_view>
 #include <utility>
+#include <vector>
 
 #include <fmt/compile.h>
 #include <fmt/format.h>
@@ -21,7 +22,6 @@
 
 #include "libtransmission/benc.h"
 #include "libtransmission/quark.h"
-#include "libtransmission/tr-buffer.h"
 #include "libtransmission/utils.h"
 #include "libtransmission/variant.h"
 
@@ -275,65 +275,99 @@ namespace
 {
 namespace to_string_helpers
 {
-using OutBuf = libtransmission::StackBuffer<1024U * 8U, std::byte>;
+using OutBuf = fmt::memory_buffer;
 
-void saveNullFunc(tr_variant const& /*var*/, std::nullptr_t /*val*/, void* vout)
+[[nodiscard]] auto sorted_entries(tr_variant::Map const& map)
 {
-    static_cast<OutBuf*>(vout)->add("0:"sv);
+    auto entries = std::vector<std::pair<std::string_view, tr_variant const*>>{};
+    entries.reserve(map.size());
+    for (auto const& [key, child] : map)
+    {
+        entries.emplace_back(tr_quark_get_string_view(key), &child);
+    }
+
+    std::sort(std::begin(entries), std::end(entries));
+    return entries;
 }
 
-void saveIntFunc(tr_variant const& /*var*/, int64_t const val, void* vout)
+struct BencWriter
 {
-    auto out = static_cast<OutBuf*>(vout);
+    void operator()(std::monostate /*unused*/) const
+    {
+    }
 
-    auto const [buf, buflen] = out->reserve_space(64U);
+    void operator()(std::nullptr_t) const
-    auto* walk = reinterpret_cast<char*>(buf);
+    {
-    auto const* const begin = walk;
+        write_string(""sv);
-    walk = fmt::format_to(walk, FMT_COMPILE("i{:d}e"), val);
+    }
-    out->commit_space(walk - begin);
-}
 
-void saveBoolFunc(tr_variant const& /*var*/, bool const val, void* vout)
+    void operator()(bool val) const
-{
+    {
-    static_cast<OutBuf*>(vout)->add(val ? "i1e"sv : "i0e"sv);
+        append_literal(val ? "i1e"sv : "i0e"sv);
-}
+    }
 
-void saveStringImpl(OutBuf* out, std::string_view sv)
+    void operator()(int64_t val) const
-{
+    {
-    // `${sv.size()}:${sv}`
+        write_int(val);
-    auto const [buf, buflen] = out->reserve_space(std::size(sv) + 32U);
+    }
-    auto* begin = reinterpret_cast<char*>(buf);
-    auto* const end = fmt::format_to(begin, FMT_COMPILE("{:d}:{:s}"), std::size(sv), sv);
-    out->commit_space(end - begin);
-}
 
-void saveStringFunc(tr_variant const& /*var*/, std::string_view const val, void* vout)
+    void operator()(double val) const
-{
+    {
-    saveStringImpl(static_cast<OutBuf*>(vout), val);
+        write_real(val);
-}
+    }
 
-void saveRealFunc(tr_variant const& /*val*/, double const val, void* vout)
+    void operator()(std::string_view sv) const
-{
+    {
-    // the benc spec doesn't handle floats; save it as a string.
+        write_string(sv);
+    }
+
+    void operator()(tr_variant::Vector const& vec) const
+    {
+        out_.push_back('l');
+        for (auto const& child : vec)
+        {
+            child.visit(*this);
+        }
+        out_.push_back('e');
+    }
+
+    void operator()(tr_variant::Map const& map) const
+    {
+        out_.push_back('d');
+        auto entries = sorted_entries(map);
+        for (auto const& [key, child] : entries)
+        {
+            write_string(key);
+            child->visit(*this);
+        }
+        out_.push_back('e');
+    }
+
+    OutBuf& out_;
+
+private:
+    void write_string(std::string_view sv) const
+    {
+        fmt::format_to(fmt::appender(out_), "{:d}:{:s}", std::size(sv), sv);
+    }
+
+    void write_int(int64_t val) const
+    {
+        fmt::format_to(fmt::appender(out_), "i{:d}e", val);
+    }
+
+    void write_real(double val) const
+    {
         auto buf = std::array<char, 64>{};
-    auto const* const out = fmt::format_to(std::data(buf), FMT_COMPILE("{:f}"), val);
+        auto const* const out_ptr = fmt::format_to(std::data(buf), "{:f}", val);
-    saveStringImpl(static_cast<OutBuf*>(vout), { std::data(buf), static_cast<size_t>(out - std::data(buf)) });
+        write_string({ std::data(buf), static_cast<size_t>(out_ptr - std::data(buf)) });
-}
+    }
 
-void saveDictBeginFunc(tr_variant const& /*val*/, void* vbuf)
+    void append_literal(std::string_view literal) const
-{
+    {
-    static_cast<OutBuf*>(vbuf)->push_back('d');
+        out_.append(std::data(literal), std::data(literal) + std::size(literal));
-}
+    }
-
+};
-void saveListBeginFunc(tr_variant const& /*val*/, void* vbuf)
-{
-    static_cast<OutBuf*>(vbuf)->push_back('l');
-}
-
-void saveContainerEndFunc(tr_variant const& /*val*/, void* vbuf)
-{
-    static_cast<OutBuf*>(vbuf)->push_back('e');
-}
 
 } // namespace to_string_helpers
 } // namespace
@@ -342,18 +376,7 @@ std::string tr_variant_serde::to_benc_string(tr_variant const& var)
 {
     using namespace to_string_helpers;
 
-    static auto constexpr Funcs = WalkFuncs{
-        saveNullFunc, //
-        saveIntFunc, //
-        saveBoolFunc, //
-        saveRealFunc, //
-        saveStringFunc, //
-        saveDictBeginFunc, //
-        saveListBeginFunc, //
-        saveContainerEndFunc, //
-    };
-
     auto buf = OutBuf{};
-    walk(var, Funcs, &buf, true);
+    var.visit(BencWriter{ buf });
-    return buf.to_string();
+    return fmt::to_string(buf);
 }

libtransmission/variant-json.cc

@@ -15,6 +15,7 @@
 #include <type_traits>
 #include <utility>
 #include <variant>
+#include <vector>
 
 #include <fmt/format.h>
 
@@ -95,10 +96,15 @@ struct json_to_variant_handler : public rapidjson::BaseReaderHandler<>
 
     bool StartObject()
     {
-        tr_variantInitDict(push_stack(), prealloc_guess());
+        if (auto* node = push_stack())
+        {
+            tr_variantInitDict(node, prealloc_guess());
             return true;
         }
 
+        return false;
+    }
+
     bool Key(Ch const* const str, rapidjson::SizeType const len, bool const copy)
     {
         if (copy)
@@ -121,10 +127,15 @@ struct json_to_variant_handler : public rapidjson::BaseReaderHandler<>
 
     bool StartArray()
     {
-        tr_variantInitList(push_stack(), prealloc_guess());
+        if (auto* node = push_stack())
+        {
+            tr_variantInitList(node, prealloc_guess());
             return true;
         }
 
+        return false;
+    }
+
     bool EndArray(rapidjson::SizeType const len)
     {
         pop_stack(len);
@@ -140,7 +151,7 @@ private:
 
     tr_variant* push_stack() noexcept
     {
-        return stack_.emplace(get_leaf());
+        return std::size(stack_) < MaxDepth ? stack_.emplace(get_leaf()) : nullptr;
     }
 
     void pop_stack(rapidjson::SizeType const len) noexcept
@@ -228,11 +239,14 @@ std::optional<tr_variant> tr_variant_serde::parse_json(std::string_view input)
     {
         return std::optional<tr_variant>{ std::move(top) };
     }
-
     if (auto err_code = reader.GetParseErrorCode(); err_code == rapidjson::kParseErrorDocumentEmpty)
     {
         error_.set(EINVAL, "No content");
     }
+    else if (err_code == rapidjson::kParseErrorTermination)
+    {
+        error_.set(E2BIG, "Max stack depth reached; unable to continue parsing");
+    }
     else
     {
         error_.set(
@@ -254,112 +268,101 @@ namespace
 {
 namespace to_string_helpers
 {
-// implements RapidJSON's Stream concept, so that the library can output
+// implements RapidJSON's write-only stream concept using fmt::memory_buffer.
-// directly to a std::string, and we can avoid some copying by copy elision
+// See <rapidjson/stream.h> for details.
-// http://rapidjson.org/md_doc_stream.html
+struct FmtOutputStream
-struct string_output_stream
 {
     using Ch = char;
 
-    explicit string_output_stream(std::string& str)
+    void Put(Ch const ch)
-        : str_ref_{ str }
+    {
+        buf_.push_back(ch);
+    }
+
+    void Flush()
     {
     }
 
-    [[nodiscard]] static Ch Peek()
+    [[nodiscard]] std::string to_string() const
     {
-        TR_ASSERT(false);
+        return fmt::to_string(buf_);
-        return 0;
-    }
-
-    [[nodiscard]] static Ch Take()
-    {
-        TR_ASSERT(false);
-        return 0;
-    }
-
-    static size_t Tell()
-    {
-        TR_ASSERT(false);
-        return 0U;
-    }
-
-    static Ch* PutBegin()
-    {
-        TR_ASSERT(false);
-        return nullptr;
-    }
-
-    void Put(Ch const c)
-    {
-        str_ref_ += c;
-    }
-
-    static void Flush()
-    {
-    }
-
-    static size_t PutEnd(Ch* /*begin*/)
-    {
-        TR_ASSERT(false);
-        return 0U;
     }
 
 private:
-    std::string& str_ref_;
+    fmt::memory_buffer buf_;
 };
 
-using writer_var_t = std::variant<rapidjson::Writer<string_output_stream>, rapidjson::PrettyWriter<string_output_stream>>;
+[[nodiscard]] auto sorted_entries(tr_variant::Map const& map)
-
-void jsonNullFunc(tr_variant const& /*var*/, std::nullptr_t /*val*/, void* vdata)
 {
-    std::visit([](auto&& writer) { writer.Null(); }, *static_cast<writer_var_t*>(vdata));
+    auto entries = std::vector<std::pair<std::string_view, tr_variant const*>>{};
-}
+    entries.reserve(map.size());
-
+    for (auto const& [key, child] : map)
-void jsonIntFunc(tr_variant const& /*var*/, int64_t const val, void* vdata)
-{
-    std::visit([val](auto&& writer) { writer.Int64(val); }, *static_cast<writer_var_t*>(vdata));
-}
-
-void jsonBoolFunc(tr_variant const& /*var*/, bool const val, void* vdata)
-{
-    std::visit([val](auto&& writer) { writer.Bool(val); }, *static_cast<writer_var_t*>(vdata));
-}
-
-void jsonRealFunc(tr_variant const& /*var*/, double const val, void* vdata)
-{
-    std::visit([val](auto&& writer) { writer.Double(val); }, *static_cast<writer_var_t*>(vdata));
-}
-
-void jsonStringFunc(tr_variant const& /*var*/, std::string_view sv, void* vdata)
-{
-    std::visit([sv](auto&& writer) { writer.String(std::data(sv), std::size(sv)); }, *static_cast<writer_var_t*>(vdata));
-}
-
-void jsonDictBeginFunc(tr_variant const& /*var*/, void* vdata)
-{
-    std::visit([](auto&& writer) { writer.StartObject(); }, *static_cast<writer_var_t*>(vdata));
-}
-
-void jsonListBeginFunc(tr_variant const& /*var*/, void* vdata)
-{
-    std::visit([](auto&& writer) { writer.StartArray(); }, *static_cast<writer_var_t*>(vdata));
-}
-
-void jsonContainerEndFunc(tr_variant const& var, void* vdata)
-{
-    auto& writer_var = *static_cast<writer_var_t*>(vdata);
-
-    if (var.holds_alternative<tr_variant::Map>())
     {
-        std::visit([](auto&& writer) { writer.EndObject(); }, writer_var);
+        entries.emplace_back(tr_quark_get_string_view(key), &child);
-    }
-    else /* list */
-    {
-        std::visit([](auto&& writer) { writer.EndArray(); }, writer_var);
     }
+    std::sort(std::begin(entries), std::end(entries));
+    return entries;
 }
 
+template<typename WriterT>
+struct JsonWriter
+{
+    WriterT& writer;
+
+    void operator()(std::monostate /*unused*/) const
+    {
+    }
+
+    void operator()(std::nullptr_t) const
+    {
+        writer.Null();
+    }
+
+    void operator()(bool const val) const
+    {
+        writer.Bool(val);
+    }
+
+    void operator()(int64_t const val) const
+    {
+        writer.Int64(val);
+    }
+
+    void operator()(double const val) const
+    {
+        writer.Double(val);
+    }
+
+    void operator()(std::string_view const val) const
+    {
+        writer.String(std::data(val), std::size(val));
+    }
+
+    void operator()(tr_variant::Vector const& val) const
+    {
+        writer.StartArray();
+        for (auto const& child : val)
+        {
+            child.visit(*this);
+        }
+        writer.EndArray();
+    }
+
+    void operator()(tr_variant::Map const& val) const
+    {
+        writer.StartObject();
+        for (auto const& [key, child] : sorted_entries(val))
+        {
+            writer.Key(std::data(key), std::size(key));
+            child->visit(*this);
+        }
+        writer.EndObject();
+    }
+};
+
+template<typename WriterT>
+JsonWriter(WriterT&) -> JsonWriter<WriterT>;
+
 } // namespace to_string_helpers
 } // namespace
 
@@ -367,30 +370,16 @@ std::string tr_variant_serde::to_json_string(tr_variant const& var) const
 {
     using namespace to_string_helpers;
 
-    static auto constexpr Funcs = WalkFuncs{
+    auto buf = FmtOutputStream{};
-        jsonNullFunc, //
-        jsonIntFunc, //
-        jsonBoolFunc, //
-        jsonRealFunc, //
-        jsonStringFunc, //
-        jsonDictBeginFunc, //
-        jsonListBeginFunc, //
-        jsonContainerEndFunc, //
-    };
-
-    auto out = std::string{};
-    out.reserve(rapidjson::StringBuffer::kDefaultCapacity);
-    auto stream = string_output_stream{ out };
-    auto writer = writer_var_t{};
     if (compact_)
     {
-        writer.emplace<0>(stream);
+        auto writer = rapidjson::Writer{ buf };
+        var.visit(JsonWriter{ writer });
     }
     else
     {
-        writer.emplace<1>(stream);
+        auto writer = rapidjson::PrettyWriter{ buf };
+        var.visit(JsonWriter{ writer });
     }
-    walk(var, Funcs, &writer, true);
+    return buf.to_string();
-
-    return out;
 }

libtransmission/variant.cc

@@ -3,13 +3,13 @@
 // or any future license endorsed by Mnemosyne LLC.
 // License text can be found in the licenses/ folder.
 
-#include <algorithm> // std::sort
 #include <cstddef>
 #include <cstdint>
 #include <iterator>
 #include <optional>
 #include <string>
 #include <string_view>
+#include <type_traits>
 #include <utility>
 #include <vector>
 #include <variant>
@@ -20,8 +20,6 @@
 
 #include <fmt/format.h>
 
-#include <small/vector.hpp>
-
 #define LIBTRANSMISSION_VARIANT_MODULE
 
 #include "libtransmission/error.h"
@@ -35,21 +33,6 @@ using namespace std::literals;
 
 namespace
 {
-[[nodiscard]] constexpr bool variant_is_container(tr_variant const* const var)
-{
-    return var != nullptr && (var->holds_alternative<tr_variant::Vector>() || var->holds_alternative<tr_variant::Map>());
-}
-
-[[nodiscard]] constexpr size_t variant_index(tr_variant const* const var)
-{
-    if (var != nullptr)
-    {
-        return var->index();
-    }
-
-    return tr_variant::NoneIndex;
-}
-
 template<typename T>
 [[nodiscard]] bool value_if(tr_variant const* const var, T* const setme)
 {
@@ -199,63 +182,55 @@ tr_variant::StringHolder& tr_variant::StringHolder::operator=(StringHolder&& tha
 
 // ---
 
-tr_variant::Merge::Merge(tr_variant& tgt)
+tr_variant tr_variant::clone() const
-    : tgt_{ tgt }
 {
+    auto ret = tr_variant{};
+    ret.merge(*this);
+    return ret;
 }
 
-void tr_variant::Merge::operator()(std::monostate const& src)
+tr_variant& tr_variant::merge(tr_variant const& that)
 {
-    tgt_ = src;
+    that.visit(
-}
+        [this](auto const& value)
-void tr_variant::Merge::operator()(std::nullptr_t const& src)
-{
-    tgt_ = src;
-}
-void tr_variant::Merge::operator()(bool const& src)
-{
-    tgt_ = src;
-}
-void tr_variant::Merge::operator()(int64_t const& src)
-{
-    tgt_ = src;
-}
-void tr_variant::Merge::operator()(double const& src)
-{
-    tgt_ = src;
-}
-void tr_variant::Merge::operator()(tr_variant::StringHolder const& src)
-{
-    tgt_ = src.sv_;
-}
-
-void tr_variant::Merge::operator()(tr_variant::Vector const& src)
-{
-    auto const n_items = std::size(src);
-    auto& tgt = tgt_.val_.emplace<Vector>();
-    tgt.resize(n_items);
-    for (size_t i = 0; i < n_items; ++i)
         {
-        std::visit(Merge{ tgt[i] }, src[i].val_);
+            using ValueType = std::decay_t<decltype(value)>;
+
+            if constexpr (
+                std::is_same_v<ValueType, std::monostate> || std::is_same_v<ValueType, std::nullptr_t> ||
+                std::is_same_v<ValueType, bool> || std::is_same_v<ValueType, int64_t> || std::is_same_v<ValueType, double> ||
+                std::is_same_v<ValueType, std::string_view>)
+            {
+                *this = value;
             }
-}
+            else if constexpr (std::is_same_v<ValueType, Vector>)
-
-void tr_variant::Merge::operator()(tr_variant::Map const& src)
-{
-    // if tgt_ isn't already a map, make it one
-    if (tgt_.index() != tr_variant::MapIndex)
             {
-        tgt_.val_.emplace<tr_variant::Map>();
+                auto& dest = val_.emplace<Vector>();
+                dest.resize(std::size(value));
+                for (size_t i = 0; i < std::size(value); ++i)
+                {
+                    dest[i].merge(value[i]);
+                }
+            }
+            else if constexpr (std::is_same_v<ValueType, Map>)
+            {
+                if (index() != MapIndex)
+                {
+                    val_.emplace<Map>();
                 }
 
-    if (auto* tgt = tgt_.get_if<tr_variant::MapIndex>(); tgt != nullptr)
+                if (auto* dest = this->template get_if<MapIndex>(); dest != nullptr)
                 {
-        tgt->reserve(std::size(*tgt) + std::size(src));
+                    dest->reserve(std::size(*dest) + std::size(value));
-        for (auto const& [key, val] : src)
+                    for (auto const& [key, child] : value)
                     {
-            std::visit(Merge{ (*tgt)[tr_quark_convert(key)] }, val.val_);
+                        (*dest)[tr_quark_convert(key)].merge(child);
                     }
                 }
+            }
+        });
+
+    return *this;
 }
 
 // ---
@@ -552,267 +527,6 @@ bool tr_variantDictRemove(tr_variant* const var, tr_quark key)
     return false;
 }
 
-// --- BENC WALKING
-
-class WalkNode
-{
-public:
-    WalkNode() = default;
-
-    explicit WalkNode(tr_variant const* const var)
-        : var_{ var }
-    {
-    }
-
-    std::pair<tr_quark, tr_variant const*> next_child()
-    {
-        if (var_ == nullptr)
-        {
-            return {};
-        }
-
-        if (auto const* const map = var_->get_if<tr_variant::MapIndex>(); map != nullptr)
-        {
-            if (auto idx = next_index(); idx < std::size(*map))
-            {
-                auto iter = std::cbegin(*map);
-                std::advance(iter, idx);
-                return { iter->first, &iter->second };
-            }
-        }
-        else if (auto const* const vec = var_->get_if<tr_variant::VectorIndex>(); vec != nullptr)
-        {
-            if (auto idx = next_index(); idx < std::size(*vec))
-            {
-                return { {}, &vec->at(idx) };
-            }
-        }
-
-        return {};
-    }
-
-    [[nodiscard]] constexpr auto is_visited() const noexcept
-    {
-        return is_visited_;
-    }
-
-    constexpr void set_visited() noexcept
-    {
-        is_visited_ = true;
-    }
-
-    [[nodiscard]] tr_variant const* current() const noexcept
-    {
-        return var_;
-    }
-
-protected:
-    friend class VariantWalker;
-
-    tr_variant const* var_ = nullptr;
-
-    bool is_visited_ = false;
-
-    void assign(tr_variant const* v_in)
-    {
-        var_ = v_in;
-        is_visited_ = false;
-        child_index_ = 0;
-        sorted_.clear();
-    }
-
-    struct ByKey
-    {
-        std::string_view key;
-        size_t idx = {};
-    };
-
-    template<typename Container>
-    void sort(Container& sortbuf)
-    {
-        auto const* const map = var_ != nullptr ? var_->get_if<tr_variant::MapIndex>() : nullptr;
-        if (map == nullptr)
-        {
-            return;
-        }
-
-        auto idx = size_t{};
-        auto const n = std::size(*map);
-        sortbuf.resize(n);
-        for (auto const& [key, val] : *map)
-        {
-            sortbuf[idx] = { tr_quark_get_string_view(key), idx };
-            ++idx;
-        }
-
-        std::sort(std::begin(sortbuf), std::end(sortbuf), [](ByKey const& a, ByKey const& b) { return a.key < b.key; });
-
-        //  keep the sorted indices
-
-        sorted_.resize(n);
-        for (size_t i = 0; i < n; ++i)
-        {
-            sorted_[i] = sortbuf[i].idx;
-        }
-    }
-
-private:
-    // When walking `v`'s children, this is the index of the next child
-    size_t child_index_ = 0;
-
-    // When `v` is a dict, this is its children's indices sorted by key.
-    // Bencoded dicts must be sorted, so this is useful when writing benc.
-    small::vector<size_t, 128U> sorted_;
-
-    [[nodiscard]] size_t next_index()
-    {
-        auto idx = child_index_++;
-
-        if (idx < std::size(sorted_))
-        {
-            idx = sorted_[idx];
-        }
-
-        return idx;
-    }
-};
-
-class VariantWalker
-{
-public:
-    void emplace(tr_variant const* v_in, bool sort_dicts)
-    {
-        stack_.emplace_back(v_in);
-
-        if (sort_dicts)
-        {
-            top().sort(sortbuf_);
-        }
-    }
-
-    void pop()
-    {
-        TR_ASSERT(!std::empty(stack_));
-
-        if (auto const size = std::size(stack_); size != 0U)
-        {
-            stack_.resize(size - 1U);
-        }
-    }
-
-    [[nodiscard]] bool empty() const noexcept
-    {
-        return std::empty(stack_);
-    }
-
-    WalkNode& top()
-    {
-        TR_ASSERT(!std::empty(stack_));
-        return stack_.back();
-    }
-
-private:
-    static auto constexpr InitialCapacity = size_t{ 24U };
-    small::vector<WalkNode, InitialCapacity> stack_;
-    small::vector<WalkNode::ByKey, InitialCapacity> sortbuf_;
-};
-
-/**
- * This function's previous recursive implementation was
- * easier to read, but was vulnerable to a smash-stacking
- * attack via maliciously-crafted data. (#667)
- */
-void tr_variant_serde::walk(tr_variant const& top, WalkFuncs const& walk_funcs, void* user_data, bool sort_dicts)
-{
-    auto stack = VariantWalker{};
-    stack.emplace(&top, sort_dicts);
-
-    while (!stack.empty())
-    {
-        auto& node = stack.top();
-        tr_variant const* v = nullptr;
-
-        if (!node.is_visited())
-        {
-            v = node.current();
-            node.set_visited();
-        }
-        else
-        {
-            auto [key, child] = node.next_child();
-
-            v = child;
-
-            if (v != nullptr)
-            {
-                if (node.current()->holds_alternative<tr_variant::Map>())
-                {
-                    auto const keystr = tr_quark_get_string_view(key);
-                    walk_funcs.string_func(tr_variant::unmanaged_string(keystr), keystr, user_data);
-                }
-            }
-            else // finished with this node
-            {
-                if (variant_is_container(node.current()))
-                {
-                    walk_funcs.container_end_func(*node.current(), user_data);
-                }
-
-                stack.pop();
-                continue;
-            }
-        }
-
-        switch (variant_index(v))
-        {
-        case tr_variant::NullIndex:
-            walk_funcs.null_func(*v, *v->get_if<tr_variant::NullIndex>(), user_data);
-            break;
-
-        case tr_variant::BoolIndex:
-            walk_funcs.bool_func(*v, *v->get_if<tr_variant::BoolIndex>(), user_data);
-            break;
-
-        case tr_variant::IntIndex:
-            walk_funcs.int_func(*v, *v->get_if<tr_variant::IntIndex>(), user_data);
-            break;
-
-        case tr_variant::DoubleIndex:
-            walk_funcs.double_func(*v, *v->get_if<tr_variant::DoubleIndex>(), user_data);
-            break;
-
-        case tr_variant::StringIndex:
-            walk_funcs.string_func(*v, *v->get_if<tr_variant::StringIndex>(), user_data);
-            break;
-
-        case tr_variant::VectorIndex:
-            if (v == node.current())
-            {
-                walk_funcs.list_begin_func(*v, user_data);
-            }
-            else
-            {
-                stack.emplace(v, sort_dicts);
-            }
-            break;
-
-        case tr_variant::MapIndex:
-            if (v == node.current())
-            {
-                walk_funcs.dict_begin_func(*v, user_data);
-            }
-            else
-            {
-                stack.emplace(v, sort_dicts);
-            }
-            break;
-
-        default: // NoneIndex:
-            break;
-        }
-    }
-}
-
 // ---
 
 bool tr_variantDictChild(tr_variant* const var, size_t pos, tr_quark* key, tr_variant** setme_value)

libtransmission/variant.h

@@ -8,6 +8,7 @@
 #include <algorithm> // std::move()
 #include <cstddef> // size_t
 #include <cstdint> // int64_t
+#include <functional> // std::invoke
 #include <initializer_list>
 #include <optional>
 #include <string>
@@ -250,11 +251,15 @@ public:
 
     constexpr tr_variant() noexcept = default;
     ~tr_variant() = default;
-    tr_variant(tr_variant const&) = delete;
     tr_variant(tr_variant&& that) noexcept = default;
-    tr_variant& operator=(tr_variant const&) = delete;
     tr_variant& operator=(tr_variant&& that) noexcept = default;
 
+    // Copying a variant is potentially expensive, so copy assignment
+    // and copy construct are deleted here to prevent accidental copies.
+    // Use clone() instead.
+    tr_variant(tr_variant const&) = delete;
+    tr_variant& operator=(tr_variant const&) = delete;
+
     template<typename Val>
     tr_variant(Val&& value) // NOLINT(bugprone-forwarding-reference-overload, google-explicit-constructor)
     {
@@ -430,12 +435,28 @@ public:
         val_.emplace<std::monostate>();
     }
 
-    tr_variant& merge(tr_variant const& that)
+    template<typename Visitor>
+    [[nodiscard]] constexpr decltype(auto) visit(Visitor&& visitor)
     {
-        std::visit(Merge{ *this }, that.val_);
+        return std::visit(make_visit_adapter(std::forward<Visitor>(visitor)), val_);
-        return *this;
     }
 
+    template<typename Visitor>
+    [[nodiscard]] constexpr decltype(auto) visit(Visitor&& visitor) const
+    {
+        return std::visit(make_visit_adapter(std::forward<Visitor>(visitor)), val_);
+    }
+
+    // Usually updates `this` to hold a clone of `that`, with two exceptions:
+    // 1. If both sides hold maps, recursively merge each entry and overwrite
+    //    duplicate keys from `this`.
+    // 2. Any unmanaged string taken from `that` is copied so `this` owns its copy.
+    tr_variant& merge(tr_variant const& that);
+
+    // Returns a new copy of `this`.
+    // Any unmanaged strings in `this` are copied so the new variant owns its copy.
+    [[nodiscard]] tr_variant clone() const;
+
 private:
     // Holds a string_view to either an unmanaged/external string or to
     // one owned by the class. If the string is unmanaged, only sv_ is used.
@@ -458,23 +479,59 @@ private:
         std::string str_;
     };
 
-    class Merge
+    template<typename Visitor>
+    class VisitAdapter
     {
     public:
-        explicit Merge(tr_variant& tgt);
+        explicit constexpr VisitAdapter(Visitor visitor)
-        void operator()(std::monostate const& src);
+            : visitor_{ std::move(visitor) }
-        void operator()(std::nullptr_t const& src);
+        {
-        void operator()(bool const& src);
+        }
-        void operator()(int64_t const& src);
+
-        void operator()(double const& src);
+        // These ref-qualified overloads preserve the visitor's cv/ref category
-        void operator()(tr_variant::StringHolder const& src);
+        // (lvalue/rvalue, const/non-const) to match std::visit() semantics.
-        void operator()(tr_variant::Vector const& src);
+        template<typename T>
-        void operator()(tr_variant::Map const& src);
+        [[nodiscard]] constexpr decltype(auto) operator()(T&& value) &
+        {
+            return call(*this, std::forward<T>(value));
+        }
+        template<typename T>
+        [[nodiscard]] constexpr decltype(auto) operator()(T&& value) const&
+        {
+            return call(*this, std::forward<T>(value));
+        }
+        template<typename T>
+        [[nodiscard]] constexpr decltype(auto) operator()(T&& value) &&
+        {
+            return call(std::move(*this), std::forward<T>(value));
+        }
 
     private:
-        tr_variant& tgt_;
+        template<typename Self, typename T>
+        [[nodiscard]] static constexpr decltype(auto) call(Self&& self, T&& value)
+        {
+            auto&& visitor = std::forward<Self>(self).visitor_;
+
+            if constexpr (std::is_same_v<std::decay_t<T>, StringHolder>)
+            {
+                return std::invoke(std::forward<decltype(visitor)>(visitor), value.sv_);
+            }
+            else
+            {
+                return std::invoke(std::forward<decltype(visitor)>(visitor), std::forward<T>(value));
+            }
+        }
+
+        Visitor visitor_;
     };
 
+    template<typename Visitor>
+    [[nodiscard]] static constexpr auto make_visit_adapter(Visitor&& visitor)
+    {
+        using AdaptedVisitor = VisitAdapter<Visitor>;
+        return AdaptedVisitor{ std::forward<Visitor>(visitor) };
+    }
+
     std::variant<std::monostate, std::nullptr_t, bool, int64_t, double, StringHolder, Vector, Map> val_;
 };
 
@@ -637,18 +694,6 @@ private:
         Json
     };
 
-    struct WalkFuncs
-    {
-        void (*null_func)(tr_variant const& var, std::nullptr_t val, void* user_data);
-        void (*int_func)(tr_variant const& var, int64_t val, void* user_data);
-        void (*bool_func)(tr_variant const& var, bool val, void* user_data);
-        void (*double_func)(tr_variant const& var, double val, void* user_data);
-        void (*string_func)(tr_variant const& var, std::string_view val, void* user_data);
-        void (*dict_begin_func)(tr_variant const& var, void* user_data);
-        void (*list_begin_func)(tr_variant const& var, void* user_data);
-        void (*container_end_func)(tr_variant const& var, void* user_data);
-    };
-
     explicit tr_variant_serde(Type type)
         : type_{ type }
     {
@@ -660,8 +705,6 @@ private:
     [[nodiscard]] std::string to_json_string(tr_variant const& var) const;
     [[nodiscard]] static std::string to_benc_string(tr_variant const& var);
 
-    static void walk(tr_variant const& top, WalkFuncs const& walk_funcs, void* user_data, bool sort_dicts);
-
     Type type_;
 
     bool compact_ = false;

tests/libtransmission/variant-test.cc

@@ -7,8 +7,11 @@
 #include <cerrno>
 #include <cstddef> // size_t
 #include <cstdint> // int64_t
+#include <map>
 #include <string>
 #include <string_view>
+#include <type_traits>
+#include <utility>
 #include <vector>
 
 #define LIBTRANSMISSION_VARIANT_MODULE
@@ -24,12 +27,22 @@
 
 using namespace std::literals;
 
-class VariantTest : public ::testing::Test
+using VariantTest = ::testing::Test;
+
+namespace
 {
-protected:
+
-    static void expectVariantMatchesQuark(tr_quark key);
+template<class... Ts>
+struct Overloaded : Ts...
+{
+    using Ts::operator()...;
 };
 
+template<class... Ts>
+Overloaded(Ts...) -> Overloaded<Ts...>;
+
+} // namespace
+
 #ifndef _WIN32
 #define STACK_SMASH_DEPTH (1 * 1000 * 1000)
 #else
@@ -86,33 +99,41 @@ TEST_F(VariantTest, getType)
     EXPECT_EQ(strkey, *sv);
 }
 
-// static
+TEST_F(VariantTest, mergeStringsTakesOwnership)
-void VariantTest::expectVariantMatchesQuark(tr_quark const key)
 {
-    auto const key_sv = tr_quark_get_string_view(key);
+    auto const is_equal_string = [](std::string_view const a, std::string_view const b)
+    {
+        return a == b;
+    };
 
-    auto const var = tr_variant::unmanaged_string(key);
+    auto const is_same_address = [](std::string_view const a, std::string_view const b)
-    auto const var_sv = var.value_if<std::string_view>();
+    {
-    ASSERT_TRUE(var_sv);
+        return std::data(a) == std::data(b);
+    };
 
-    // The strings should not just be equal,
+    // set up `src` to hold an unmanaged string
-    // but should point to literally the same memory
+    auto constexpr Original = "this is the string"sv;
-    EXPECT_EQ(key_sv, *var_sv);
+    auto const src = tr_variant::unmanaged_string(Original);
-    EXPECT_EQ(std::data(key_sv), std::data(*var_sv));
+    auto src_sv = src.value_if<std::string_view>().value_or(""sv);
-}
 
-TEST_F(VariantTest, unmanagedStringFromPredefinedQuark)
+    // set up `tgt` to hold another unmanaged string
-{
+    auto constexpr WillBeReplaced = "some other string"sv;
-    expectVariantMatchesQuark(TR_KEY_name);
+    static_assert(Original != WillBeReplaced);
-}
+    auto tgt = tr_variant::unmanaged_string(WillBeReplaced);
+    auto tgt_sv = tgt.value_if<std::string_view>().value_or(""sv);
 
-TEST_F(VariantTest, unmanagedStringFromNewQuark)
+    // test that `src` and `tgt` hold unmanaged strings
-{
+    EXPECT_TRUE(is_equal_string(Original, src_sv));
-    static auto constexpr NewString = std::string_view{ "this-string-is-not-already-interned" };
+    EXPECT_TRUE(is_equal_string(WillBeReplaced, tgt_sv));
-    ASSERT_FALSE(tr_quark_lookup(NewString));
+    EXPECT_TRUE(is_same_address(Original, src_sv));
+    EXPECT_TRUE(is_same_address(WillBeReplaced, tgt_sv));
 
-    auto const key = tr_quark_new(NewString);
+    tgt.merge(src);
-    expectVariantMatchesQuark(key);
+
+    // test that `tgt` now holds its own copy of `Original`.
+    auto const actual = tgt.value_if<std::string_view>().value_or(""sv);
+    EXPECT_TRUE(is_equal_string(Original, actual));
+    EXPECT_FALSE(is_same_address(Original, actual));
 }
 
 TEST_F(VariantTest, parseInt)
@@ -379,73 +400,72 @@ TEST_F(VariantTest, bencToJson)
     }
 }
 
-TEST_F(VariantTest, merge)
+TEST_F(VariantTest, mergeMapsCreatesCombinedMap)
 {
-    auto const i1 = tr_quark_new("i1"sv);
+    auto serde = tr_variant_serde::json();
-    auto const i2 = tr_quark_new("i2"sv);
+    serde.compact();
-    auto const i3 = tr_quark_new("i3"sv);
+    serde.inplace();
-    auto const i4 = tr_quark_new("i4"sv);
-    auto const s5 = tr_quark_new("s5"sv);
-    auto const s6 = tr_quark_new("s6"sv);
-    auto const s7 = tr_quark_new("s7"sv);
-    auto const s8 = tr_quark_new("s8"sv);
 
-    /* initial dictionary (default values) */
+    auto src = serde.parse(R"({"src_key":123})"sv).value_or(tr_variant{});
-    auto dest = tr_variant::make_map(6U);
+    auto tgt = serde.parse(R"({"tgt_key":456})"sv).value_or(tr_variant{});
-    auto* map = dest.get_if<tr_variant::Map>();
+    tgt.merge(src);
-    map->try_emplace(i1, 1);
+    EXPECT_EQ(R"({"src_key":123,"tgt_key":456})"sv, serde.to_string(tgt));
-    map->try_emplace(i2, 2);
-    map->try_emplace(i4, -35); /* remains untouched */
-    map->try_emplace(s5, "abc");
-    map->try_emplace(s6, "def");
-    map->try_emplace(s7, "127.0.0.1"); /* remains untouched */
-
-    /* new dictionary, will overwrite items in dest */
-    auto src = tr_variant::make_map(6U);
-    map = src.get_if<tr_variant::Map>();
-    map->try_emplace(i1, 1); /* same value */
-    map->try_emplace(i2, 4); /* new value */
-    map->try_emplace(i3, 3); /* new key:value */
-    map->try_emplace(s5, "abc"); /* same value */
-    map->try_emplace(s6, "xyz"); /* new value */
-    map->try_emplace(s8, "ghi"); /* new key:value */
-
-    dest.merge(src);
-
-    map = dest.get_if<tr_variant::Map>();
-    auto i = map->value_if<int64_t>(i1);
-    ASSERT_TRUE(i);
-    EXPECT_EQ(1, *i);
-    i = map->value_if<int64_t>(i2);
-    ASSERT_TRUE(i);
-    EXPECT_EQ(4, *i);
-    i = map->value_if<int64_t>(i3);
-    ASSERT_TRUE(i);
-    EXPECT_EQ(3, *i);
-    i = map->value_if<int64_t>(i4);
-    ASSERT_TRUE(i);
-    EXPECT_EQ(-35, *i);
-    auto sv = map->value_if<std::string_view>(s5);
-    ASSERT_TRUE(sv);
-    EXPECT_EQ("abc"sv, *sv);
-    sv = map->value_if<std::string_view>(s6);
-    ASSERT_TRUE(sv);
-    EXPECT_EQ("xyz"sv, *sv);
-    sv = map->value_if<std::string_view>(s7);
-    ASSERT_TRUE(sv);
-    EXPECT_EQ("127.0.0.1"sv, *sv);
-    sv = map->value_if<std::string_view>(s8);
-    ASSERT_TRUE(sv);
-    EXPECT_EQ("ghi"sv, *sv);
 }
 
-TEST_F(VariantTest, stackSmash)
+TEST_F(VariantTest, mergeMapsOverwritesSrcMapEntries)
 {
-    // make a nested list of list of lists.
+    auto serde = tr_variant_serde::json();
+    serde.compact();
+    serde.inplace();
+
+    auto src = serde.parse(R"({"src_key": 123, "dup_key":789})"sv).value_or(tr_variant{});
+    auto tgt = serde.parse(R"({"tgt_key": 456, "dup_key":456})"sv).value_or(tr_variant{});
+    tgt.merge(src);
+    EXPECT_EQ(R"({"dup_key":789,"src_key":123,"tgt_key":456})"sv, serde.to_string(tgt));
+}
+
+TEST_F(VariantTest, mergeOverwritesDifferingTypes)
+{
+    auto const variants = std::array<std::pair<tr_variant, std::string_view>, 7U>{ {
+        { tr_variant{ true }, "true" },
+        { tr_variant{ int64_t{ 123 } }, "123" },
+        { tr_variant{ 4.5 }, "4.5" },
+        { tr_variant{ "foo"sv }, R"("foo")"sv },
+        { tr_variant{ nullptr }, "null"sv },
+        { tr_variant::make_map(0U), "{}"sv },
+        { tr_variant::make_vector(), "[]"sv },
+    } };
+
+    auto serde = tr_variant_serde::json();
+    serde.compact();
+    serde.inplace();
+
+    for (auto const& [src, src_expected] : variants)
+    {
+        for (auto const& [tgt, tgt_expected] : variants)
+        {
+            if (&src != &tgt)
+            {
+                // set up `var` to be a copy of `src`
+                auto var = src.clone();
+                EXPECT_EQ(src_expected, serde.to_string(var));
+
+                var.merge(tgt);
+
+                // test that `var` is now a copy of `tgt`
+                EXPECT_EQ(tgt_expected, serde.to_string(var));
+            }
+        }
+    }
+}
+
+TEST_F(VariantTest, stackSmashBenc)
+{
+    // set up a nested list of list of lists.
     static int constexpr Depth = STACK_SMASH_DEPTH;
     std::string const in = std::string(Depth, 'l') + std::string(Depth, 'e');
 
-    // confirm that it fails instead of crashing
+    // test that parsing fails without crashing
     auto serde = tr_variant_serde::benc();
     auto var = serde.inplace().parse(in);
     EXPECT_FALSE(var.has_value());
@@ -453,7 +473,23 @@ TEST_F(VariantTest, stackSmash)
     EXPECT_EQ(E2BIG, serde.error_.code());
 }
 
-TEST_F(VariantTest, boolAndIntRecast)
+TEST_F(VariantTest, stackSmashJson)
+{
+    auto serde = tr_variant_serde::json();
+    serde.inplace();
+
+    // set up a nested array of arrays of arrays.
+    static auto constexpr Depth = STACK_SMASH_DEPTH;
+    auto const in = std::string(Depth, '[') + std::string(Depth, ']');
+
+    // test that parsing fails without crashing
+    auto var = serde.inplace().parse(in);
+    EXPECT_FALSE(var.has_value());
+    EXPECT_TRUE(serde.error_);
+    EXPECT_EQ(E2BIG, serde.error_.code());
+}
+
+TEST_F(VariantTest, valueIfCanReadBoolsAndIntsInterchangeably)
 {
     auto const key1 = tr_quark_new("key1"sv);
     auto const key2 = tr_quark_new("key2"sv);
@@ -558,134 +594,159 @@ TEST_F(VariantTest, dictFindType)
 
 TEST_F(VariantTest, mapContains)
 {
-    auto const key_bool = tr_quark_new("contains-bool"sv);
+    auto serde = tr_variant_serde::json();
-    auto const key_int = tr_quark_new("contains-int"sv);
+    serde.inplace();
-    auto const key_double = tr_quark_new("contains-double"sv);
+    serde.compact();
-    auto const key_string = tr_quark_new("contains-string"sv);
-    auto const key_vector = tr_quark_new("contains-vector"sv);
-    auto const key_map = tr_quark_new("contains-map"sv);
-    auto const key_missing = tr_quark_new("contains-missing"sv);
-    auto const nested_key = tr_quark_new("contains-nested"sv);
 
-    // populate a test map
+    // set up a map with some sample entries
-
+    static auto constexpr Input = R"({
-    auto top = tr_variant::make_map(6U);
+        "id": 42,
+        "is_finished": true,
+        "labels": ["a", "b"],
+        "units": { "speed_units": ["KB/s", "MB/s", "GB/s", "TB/s"] },
+        "upload_ratio": 4.2,
+        "version": "5.0"
+    })"sv;
+    auto top = serde.parse(Input).value_or(tr_variant{});
     auto* const map = top.get_if<tr_variant::Map>();
-    ASSERT_NE(map, nullptr);
+    ASSERT_NE(nullptr, map);
 
-    map->try_emplace(key_bool, true);
+    // test that contains() returns true for entries that exist
-    map->try_emplace(key_int, int64_t{ 42 });
+    EXPECT_TRUE(map->contains(TR_KEY_id));
-    map->try_emplace(key_double, 4.2);
+    EXPECT_TRUE(map->contains(TR_KEY_is_finished));
-    map->try_emplace(key_string, "needle"sv);
+    EXPECT_TRUE(map->contains(TR_KEY_labels));
+    EXPECT_TRUE(map->contains(TR_KEY_units));
+    EXPECT_TRUE(map->contains(TR_KEY_upload_ratio));
+    EXPECT_TRUE(map->contains(TR_KEY_version));
 
-    auto vec = tr_variant::Vector{};
+    // test that contains() returns false for entries that never existed
-    vec.emplace_back(true);
+    EXPECT_FALSE(map->contains(TR_KEY_umask));
-    vec.emplace_back(int64_t{ 7 });
-    map->try_emplace(key_vector, std::move(vec));
 
-    auto nested = tr_variant::make_map(1U);
+    // test that contains() returns false for entries that were removed
-    auto* nested_map = nested.get_if<tr_variant::Map>();
+    auto const key = TR_KEY_labels;
-    ASSERT_NE(nested_map, nullptr);
+    EXPECT_TRUE(map->contains(key));
-    nested_map->try_emplace(nested_key, "nested"sv);
+    EXPECT_EQ(1U, map->erase(key));
-    map->try_emplace(key_map, std::move(nested));
+    EXPECT_FALSE(map->contains(key));
-
-    // ---
-
-    // test: returns true for entries that exist
-    EXPECT_TRUE(map->contains(key_bool));
-    EXPECT_TRUE(map->contains(key_double));
-    EXPECT_TRUE(map->contains(key_int));
-    EXPECT_TRUE(map->contains(key_map));
-    EXPECT_TRUE(map->contains(key_string));
-    EXPECT_TRUE(map->contains(key_vector));
-
-    // test: returns false for entries that never existed
-    EXPECT_FALSE(map->contains(key_missing));
-
-    // test: returns false for entries that were removed
-    EXPECT_EQ(1U, map->erase(key_vector));
-    EXPECT_FALSE(map->contains(key_vector));
 }
 
-TEST_F(VariantTest, mapReplaceKey)
+TEST_F(VariantTest, visitStringExposesStringView)
 {
-    auto constexpr IntVal = int64_t{ 73 };
+    static auto const Text = "visit-string"sv;
-    auto const key_bool = tr_quark_new("replace-bool"sv);
+    auto var = tr_variant{ std::string{ Text } };
-    auto const key_int = tr_quark_new("replace-int"sv);
+    auto called = false;
-    auto const key_double = tr_quark_new("replace-double"sv);
-    auto const key_string = tr_quark_new("replace-string"sv);
-    auto const key_vector = tr_quark_new("replace-vector"sv);
-    auto const key_map = tr_quark_new("replace-map"sv);
-    auto const key_duplicate = tr_quark_new("replace-duplicate"sv);
-    auto const key_missing_src = tr_quark_new("replace-missing-src"sv);
-    auto const key_missing_tgt = tr_quark_new("replace-missing-tgt"sv);
-    auto const key_replacement = tr_quark_new("replace-string-new"sv);
-    auto const key_nested = tr_quark_new("replace-nested"sv);
 
-    // populate a sample map
+    var.visit(
+        Overloaded{ [&](std::string_view sv)
+                    {
+                        called = true;
+                        EXPECT_EQ(Text, sv);
+                    },
+                    [](auto&&)
+                    {
+                        FAIL();
+                    } });
 
-    auto top = tr_variant::make_map(7U);
+    EXPECT_TRUE(called);
-    auto* const map = top.get_if<tr_variant::Map>();
+}
-    ASSERT_NE(map, nullptr);
 
-    map->try_emplace(key_bool, true);
+TEST_F(VariantTest, visitConstVariant)
-    map->try_emplace(key_int, IntVal);
+{
-    map->try_emplace(key_double, 7.3);
+    auto var = tr_variant::make_vector(1U);
-    map->try_emplace(key_string, "string"sv);
+    auto* vec = var.get_if<tr_variant::Vector>();
+    ASSERT_NE(vec, nullptr);
+    vec->emplace_back(int64_t{ 99 });
 
-    auto vec = tr_variant::Vector{};
+    auto const result = std::as_const(var).visit(
-    vec.emplace_back(false);
+        Overloaded{ [](tr_variant::Vector const& values) -> int64_t
-    vec.emplace_back(int64_t{ 99 });
+                    {
-    map->try_emplace(key_vector, std::move(vec));
+                        EXPECT_EQ(1U, std::size(values));
+                        return values[0].value_if<int64_t>().value_or(-1);
+                    },
+                    [](auto&&) -> int64_t
+                    {
+                        ADD_FAILURE() << "unexpected alternative";
+                        return -1;
+                    } });
 
-    auto nested = tr_variant::make_map(1U);
+    EXPECT_EQ(99, result);
-    auto* nested_map = nested.get_if<tr_variant::Map>();
+}
-    ASSERT_NE(nested_map, nullptr);
-    nested_map->try_emplace(key_nested, "nested"sv);
-    map->try_emplace(key_map, std::move(nested));
 
-    map->try_emplace(key_duplicate, "occupied"sv);
+TEST_F(VariantTest, visitsNodesDepthFirst)
+{
+    auto serde = tr_variant_serde::json();
+    serde.compact();
+    serde.inplace();
 
-    // ---
+    // set up a test variant to be visited
+    static auto constexpr Input = R"({
+        "files": [
+            { "name": "file1", "size": 5, "pieces": [1, 2] },
+            { "name": "file2", "size": 7, "pieces": [] }
+        ],
+        "meta": { "active": true }
+    })"sv;
+    auto const var = serde.parse(Input).value_or(tr_variant{});
 
-    // test: neither src nor tgt exist
+    // set up some containers that we'll populate during `var.visit()`
-    auto const serde = tr_variant_serde::json();
+    auto visited_counts = std::map<size_t, size_t>{};
-    auto expected = serde.to_string(top);
+    auto flattened = tr_variant::Vector{};
-    EXPECT_FALSE(map->contains(key_missing_src));
+    flattened.reserve(64U);
-    EXPECT_FALSE(map->contains(key_missing_tgt));
-    EXPECT_FALSE(map->replace_key(key_missing_src, key_missing_tgt));
-    EXPECT_FALSE(map->contains(key_missing_src));
-    EXPECT_FALSE(map->contains(key_missing_tgt));
-    auto actual = serde.to_string(top);
-    EXPECT_EQ(expected, actual); // confirm variant is unchanged
 
-    // test: src doesn't exist
+    // set up the visitor
-    expected = serde.to_string(top);
+    auto flatten = [&](tr_variant const& node, auto const& self) -> void
-    EXPECT_FALSE(map->contains(key_missing_src));
+    {
-    EXPECT_EQ(IntVal, map->value_if<int64_t>(key_int).value_or(!IntVal));
+        ++visited_counts[node.index()];
-    EXPECT_FALSE(map->replace_key(key_missing_src, key_int));
-    EXPECT_FALSE(map->contains(key_missing_src));
-    EXPECT_EQ(IntVal, map->value_if<int64_t>(key_int).value_or(!IntVal));
-    actual = serde.to_string(top);
-    EXPECT_EQ(expected, actual); // confirm variant is unchanged
 
-    // test: tgt already exists
+        node.visit(
-    expected = serde.to_string(top);
+            [&](auto const& val)
-    EXPECT_TRUE(map->contains(key_int));
+            {
-    EXPECT_TRUE(map->contains(key_string));
+                using ValueType = std::decay_t<decltype(val)>;
-    EXPECT_FALSE(map->replace_key(key_int, key_string));
-    EXPECT_TRUE(map->contains(key_int));
-    EXPECT_TRUE(map->contains(key_string));
-    actual = serde.to_string(top);
-    EXPECT_EQ(expected, actual); // confirm variant is unchanged
 
-    // test: successful replacement
+                if constexpr (
-    EXPECT_TRUE(map->contains(key_int));
+                    std::is_same_v<ValueType, bool> || //
-    EXPECT_FALSE(map->contains(key_replacement));
+                    std::is_same_v<ValueType, double> || //
-    EXPECT_TRUE(map->replace_key(key_int, key_replacement));
+                    std::is_same_v<ValueType, int64_t> || //
-    EXPECT_FALSE(map->contains(key_int));
+                    std::is_same_v<ValueType, std::monostate> || //
-    EXPECT_TRUE(map->contains(key_replacement));
+                    std::is_same_v<ValueType, std::nullptr_t> || //
-    EXPECT_EQ(IntVal, map->value_if<int64_t>(key_replacement).value_or(!IntVal));
+                    std::is_same_v<ValueType, std::string_view>)
+                {
+                    flattened.emplace_back(val);
+                }
+                else if constexpr (std::is_same_v<ValueType, tr_variant::Vector>)
+                {
+                    for (auto const& child : val)
+                    {
+                        self(child, self);
+                    }
+                }
+                else if constexpr (std::is_same_v<ValueType, tr_variant::Map>)
+                {
+                    for (auto const& [key, child] : val)
+                    {
+                        flattened.emplace_back(tr_variant::unmanaged_string(key));
+                        self(child, self);
+                    }
+                }
+            });
+    };
+
+    flatten(var, flatten);
+
+    // test that the nodes were visited depth-first
+    auto const actual = serde.to_string({ std::move(flattened) });
+    auto constexpr Expected =
+        R"(["files","name","file1","size",5,"pieces",1,2,"name","file2","size",7,"pieces","meta","active",true])"sv;
+    EXPECT_EQ(Expected, actual);
+
+    // test that we visited the expected number of nodes
+    auto const expected_visited_count = std::map<size_t, size_t>{
+        { tr_variant::BoolIndex, 1U }, //
+        { tr_variant::IntIndex, 4U }, //
+        { tr_variant::MapIndex, 4U }, //
+        { tr_variant::StringIndex, 2U }, //
+        { tr_variant::VectorIndex, 3U }, //
+    };
+    EXPECT_EQ(expected_visited_count, visited_counts);
 }
 
 TEST_F(VariantTest, variantFromBufFuzz)