refactor: Let json::Value be compared and constructed from a string view

`json::Value` accepts `char const*`, `std::string` and
`json::StaticString`, so a caller holding a `std::string_view` has to
materialize a `std::string` whose characters are then copied a second
time into the value's own storage. Add the missing constructor, which
the `std::string` one delegates to, and an `operator==` that reads the
value's characters in place.

The comparison is constrained to `std::string_view` exactly rather than
taking a view by plain overload. A string literal converts equally well
to a view and to a Value, so a plain overload makes every
`value == "literal"` ambiguous, and a literal `0`, which converts to a
view through `char const*` as well as to a Value, with it.

No reply changes. The two spellings differ only for a view holding an
embedded NUL, which the Value comparison stops at. The `method: "batch"`
check in `ServerHandler.cpp` is the operator's first production user, so
that comparison stops building a `Value` from the literal on every
request.

include/xrpl/json/json_value.h

@@ -3,11 +3,13 @@
 #include <xrpl/basics/Number.h>
 #include <xrpl/json/json_forwards.h>
 
+#include <concepts>
 #include <cstring>
 #include <iterator>
 #include <limits>
 #include <map>
 #include <string>
+#include <string_view>
 #include <vector>
 
 /**
@@ -198,6 +200,15 @@ public:
      */
     Value(StaticString const& value);
     Value(std::string const& value);
+    /**
+     * @brief Constructs a value from a string view.
+     *
+     * The characters are copied, so the view need not outlive the call and need
+     * not be NUL-terminated.
+     *
+     * @param value The characters to copy.
+     */
+    Value(std::string_view value);
     Value(bool value);
     Value(Value const& other);
     ~Value();
@@ -472,6 +483,32 @@ toJson(xrpl::Number const& number)
 bool
 operator==(Value const&, Value const&);
 
+/**
+ * Compares a value with a string view, reading the value's characters in place
+ * rather than building a Value from the view.
+ *
+ * Constrained to the exact type: a string literal converts equally well to a
+ * view and to a Value, so a plain overload makes `value == "literal"`
+ * ambiguous.
+ *
+ * @param x The value to compare.
+ * @param y The characters to compare it against.
+ * @return Whether `x` is a string whose characters up to its first NUL are
+ *         exactly the characters of `y`.
+ */
+template <class T>
+    requires std::same_as<T, std::string_view>
+bool
+operator==(Value const& x, T y)
+{
+    if (!x.isString())
+        return false;
+
+    // A string `Value` can hold a null pointer, which names no characters, so it equals no view.
+    char const* const s = x.asCString();
+    return s != nullptr && std::string_view{s} == y;
+}
+
 bool
 operator<(Value const&, Value const&);
 

src/libxrpl/json/json_value.cpp

@@ -15,6 +15,7 @@
 #include <cstring>
 #include <limits>
 #include <string>
+#include <string_view>
 #include <utility>
 
 namespace json {
@@ -241,10 +242,14 @@ Value::Value(xrpl::Number const& value) : type_(ValueType::String), allocated_(t
     value_.stringVal = valueAllocator()->duplicateStringValue(tmp.c_str(), tmp.length());
 }
 
-Value::Value(std::string const& value) : type_(ValueType::String), allocated_(true)
+Value::Value(std::string const& value) : Value(std::string_view{value})
 {
-    value_.stringVal =
-        valueAllocator()->duplicateStringValue(value.c_str(), (unsigned int)value.length());
+}
+
+Value::Value(std::string_view value) : type_(ValueType::String), allocated_(true)
+{
+    value_.stringVal = valueAllocator()->duplicateStringValue(
+        value.data(), static_cast<unsigned int>(value.length()));
 }
 
 Value::Value(StaticString const& value) : type_(ValueType::String)

src/tests/libxrpl/json/Value.cpp

@@ -598,6 +598,91 @@ TEST(JsonValue, compare_strings)
 #pragma pop_macro("DO_COMPARE")
 }
 
+TEST(JsonValue, construct_from_string_view)
+{
+    using namespace std::string_view_literals;
+
+    // Characters are copied with an explicit length, so a view need not be NUL-terminated.
+    EXPECT_EQ(json::Value{"2.0"sv}.asString(), "2.0");
+    EXPECT_EQ(json::Value{"2.0 and more"sv.substr(0, 3)}.asString(), "2.0");
+    EXPECT_EQ(json::Value{""sv}.asString(), "");
+
+    // A default-constructed view has a null `data()` and yields an empty string value.
+    std::string_view const empty;
+    json::Value const fromEmpty{empty};
+    EXPECT_TRUE(fromEmpty.isString());
+    EXPECT_EQ(fromEmpty.asString(), "");
+    EXPECT_TRUE(fromEmpty == ""sv);
+
+    // The view need not outlive the call.
+    json::Value copied;
+    {
+        std::string const owner{"borrowed"};
+        copied = json::Value{std::string_view{owner}};
+    }
+    EXPECT_EQ(copied.asString(), "borrowed");
+
+    // A reader of a string value stops at the first NUL, so a view carrying one reads back short.
+    json::Value const embedded{"a\0b"sv};
+    EXPECT_EQ(embedded.asString(), "a");
+    EXPECT_TRUE(embedded != "a\0b"sv);
+    EXPECT_TRUE(embedded == "a"sv);
+    EXPECT_TRUE(embedded == json::Value{"a\0b"sv});
+}
+
+TEST(JsonValue, compare_string_view)
+{
+    // Comparing against a view builds no temporary Value.
+    using namespace std::string_view_literals;
+
+    EXPECT_TRUE(json::Value{"2.0"} == "2.0"sv);
+    EXPECT_FALSE(json::Value{"2.0"} != "2.0"sv);
+    EXPECT_TRUE(json::Value{"2.0"} != "2.00"sv);
+    EXPECT_TRUE(json::Value{"2.0"} != ""sv);
+
+    // A view need not be NUL-terminated, and the comparison must not read past its end.
+    EXPECT_TRUE(json::Value{"2.0"} == "2.0 and more"sv.substr(0, 3));
+
+    EXPECT_TRUE(json::Value{""} == ""sv);
+
+    // A string value holding a null pointer equals nothing, not even the empty view.
+    EXPECT_TRUE(json::Value{json::ValueType::String} != ""sv);
+    EXPECT_TRUE(json::Value{json::ValueType::String} != "2.0"sv);
+    EXPECT_TRUE(json::Value{json::ValueType::String} != json::Value{""});
+
+    // Nothing that is not a string equals one. Null is what an absent member reads as.
+    EXPECT_TRUE(json::Value{json::ValueType::Null} != ""sv);
+    EXPECT_TRUE(json::Value{json::ValueType::Null} != "2.0"sv);
+    EXPECT_TRUE(json::Value{2} != "2"sv);
+    EXPECT_TRUE(json::Value{true} != "true"sv);
+    EXPECT_TRUE(json::Value{json::ValueType::Object} != ""sv);
+
+    // Only a view reaches this overload; every other spelling compares through Value.
+    static constexpr json::StaticString kVersion{"2.0"};
+    EXPECT_TRUE(json::Value{"2.0"} == kVersion);
+    EXPECT_TRUE(json::Value{"2.1"} != kVersion);
+    EXPECT_TRUE(json::Value{} != kVersion);
+
+    EXPECT_TRUE(json::Value{"2.0"} == "2.0");
+    EXPECT_TRUE(json::Value{"2.0"} != "2.1");
+    EXPECT_TRUE(json::Value{} != "2.0");
+
+    EXPECT_TRUE(json::Value{"2.0"} == std::string{"2.0"});
+    EXPECT_TRUE(json::Value{"2.0"} != std::string{"2.1"});
+    EXPECT_TRUE(json::Value{json::ValueType::Null} != std::string{"2.0"});
+
+    // A literal `0` also converts to a view through `char const*`, so a plain overload would make
+    // this comparison ambiguous. It must compare as a number: RPCCall.cpp rejects a
+    // `transaction_entry` ledger index this way.
+    EXPECT_TRUE(json::Value{0} == 0);
+    EXPECT_TRUE(json::Value{4294967295u} != 0);
+    EXPECT_TRUE(json::Value{""} != 0);
+    EXPECT_TRUE(json::Value{2} != 0);
+
+    // A null and an Int are different types, so the comparison is false.
+    EXPECT_TRUE(json::Value{} != 0);
+}
+
 TEST(JsonValue, bool)
 {
     EXPECT_FALSE(json::Value());

src/xrpld/app/misc/NetworkOPs.cpp

@@ -3884,37 +3884,32 @@ NetworkOPsImp::pubProposedAccountTransaction(
     HashSet<InfoSub::pointer> notify;
     int iProposed = 0;
 
-    std::vector<SubAccountHistoryInfo> accountHistoryNotify;
-
     {
         std::scoped_lock const sl(accountLock_);
 
         if (subRTAccount_.empty())
             return;
 
-        if (!subAccount_.empty() || !subRTAccount_.empty() || !subAccountHistory_.empty())
+        for (auto const& affectedAccount : tx->getMentionedAccounts())
         {
-            for (auto const& affectedAccount : tx->getMentionedAccounts())
+            if (auto const simiIt = subRTAccount_.find(affectedAccount);
+                simiIt != subRTAccount_.end())
             {
-                if (auto const simiIt = subRTAccount_.find(affectedAccount);
-                    simiIt != subRTAccount_.end())
+                auto it = simiIt->second.begin();
+
+                while (it != simiIt->second.end())
                 {
-                    auto it = simiIt->second.begin();
+                    InfoSub::pointer const p = it->second.lock();
 
-                    while (it != simiIt->second.end())
+                    if (p)
                     {
-                        InfoSub::pointer const p = it->second.lock();
-
-                        if (p)
-                        {
-                            notify.insert(p);
-                            ++it;
-                            ++iProposed;
-                        }
-                        else
-                        {
-                            it = simiIt->second.erase(it);
-                        }
+                        notify.insert(p);
+                        ++it;
+                        ++iProposed;
+                    }
+                    else
+                    {
+                        it = simiIt->second.erase(it);
                     }
                 }
             }
@@ -3923,10 +3918,10 @@ NetworkOPsImp::pubProposedAccountTransaction(
 
     JLOG(journal_.trace()) << "pubProposedAccountTransaction: " << iProposed;
 
-    if (!notify.empty() || !accountHistoryNotify.empty())
+    if (!notify.empty())
     {
         // Create two different Json objects, for different API versions
-        MultiApiJson jvObj = transJson(tx, result, false, ledger, std::nullopt);
+        MultiApiJson const jvObj = transJson(tx, result, false, ledger, std::nullopt);
 
         for (InfoSub::Ref isrListener : notify)
         {
@@ -3934,21 +3929,6 @@ NetworkOPsImp::pubProposedAccountTransaction(
                 isrListener->getApiVersion(),  //
                 [&](json::Value const& jv) { isrListener->send(jv, true); });
         }
-
-        XRPL_ASSERT(
-            jvObj.isMember(jss::account_history_tx_stream) == MultiApiJson::IsMemberResult::None,
-            "xrpl::NetworkOPs::pubProposedAccountTransaction : "
-            "account_history_tx_stream not set");
-        for (auto& info : accountHistoryNotify)
-        {
-            auto const& index = info.index;
-            if (index->forwardTxIndex == 0 && !index->haveHistorical)
-                jvObj.set(jss::account_history_tx_first, true);
-            jvObj.set(jss::account_history_tx_index, index->forwardTxIndex++);
-            jvObj.visit(
-                info.sink->getApiVersion(),  //
-                [&](json::Value const& jv) { info.sink->send(jv, true); });
-        }
     }
 }
 

src/xrpld/rpc/detail/ServerHandler.cpp

@@ -628,7 +628,9 @@ ServerHandler::processRequest(
 
     bool batch = false;
     unsigned size = 1;
-    if (jsonOrig.isMember(jss::method) && jsonOrig[jss::method] == "batch")
+    // Spelled as a view so the name is compared in place. A bare literal would reach the
+    // Value-to-Value comparison instead, which builds a Value from it, allocating per request.
+    if (jsonOrig.isMember(jss::method) && jsonOrig[jss::method] == std::string_view{"batch"})
     {
         batch = true;
         if (!jsonOrig.isMember(jss::params) || !jsonOrig[jss::params].isArray())