Improvements to STParsedJSON:

* Cleanups and reduction of copying
* Add STArray::back, operator[], push_back(&&)
* Add make_stvar
* Rework STParsedJSON
* Fix code and unit tests that use STParsedJSON
* STTx move constructor

src/ripple/app/tests/common_ledger.cpp

@@ -112,12 +112,11 @@ STTx
 parseTransaction(TestAccount& account, Json::Value const& tx_json, bool sign)
 {
     STParsedJSONObject parsed("tx_json", tx_json);
-    std::unique_ptr<STObject> sopTrans = std::move(parsed.object);
+    if (!parsed.object)
-    if (sopTrans == nullptr)
         throw std::runtime_error(
-        "sopTrans == nullptr");
+        "object not parseable");
-    sopTrans->setFieldVL(sfSigningPubKey, account.pk.getAccountPublic());
+    parsed.object->setFieldVL(sfSigningPubKey, account.pk.getAccountPublic());
-    auto tx = STTx(*sopTrans);
+    auto tx = STTx(std::move (*parsed.object));
     if (sign)
         tx.sign(account.sk);
     return tx;

src/ripple/protocol/STArray.h

@@ -71,11 +71,37 @@ public:
         return emplace(n, buf, std::move(*this));
     }
 
-    void push_back (const STObject & object)
+    STObject& operator[] (std::size_t j)
+    {
+        return v_[j];
+    }
+
+    STObject const& operator[] (std::size_t j) const
+    {
+        return v_[j];
+    }
+
+    STObject& back() { return v_.back(); }
+
+    STObject const& back() const { return v_.back(); }
+
+    template <class... Args>
+    void
+    emplace_back(Args&&... args)
+    {
+        v_.emplace_back(std::forward<Args>(args)...);
+    }
+
+    void push_back (STObject const& object)
     {
         v_.push_back(object);
     }
 
+    void push_back (STObject&& object)
+    {
+        v_.push_back(std::move(object));
+    }
+
     iterator begin ()
     {
         return v_.begin ();
@@ -101,11 +127,6 @@ public:
         return v_.size ();
     }
 
-    STObject& back ()
-    {
-        return v_.back ();
-    }
-
     bool empty () const
     {
         return v_.empty ();

src/ripple/protocol/STObject.h

@@ -27,7 +27,6 @@
 #include <ripple/protocol/SOTemplate.h>
 #include <ripple/protocol/impl/STVar.h>
 #include <boost/iterator/transform_iterator.hpp>
-#include <boost/ptr_container/ptr_vector.hpp>
 #include <utility>
 
 #include <beast/streams/debug_ostream.h>
@@ -99,7 +98,6 @@ public:
     STObject(STObject const&) = default;
     STObject (const SOTemplate & type, SField const& name);
     STObject (const SOTemplate & type, SerialIter & sit, SField const& name);
-    STObject (SField const& name, boost::ptr_vector<STBase>& data);
     STObject (SerialIter& sit, SField const& name);
     STObject& operator= (STObject const&) = default;
     STObject& operator= (STObject&& other);

src/ripple/protocol/STParsedJSON.h

@@ -21,6 +21,7 @@
 #define RIPPLE_PROTOCOL_STPARSEDJSON_H_INCLUDED
 
 #include <ripple/protocol/STArray.h>
+#include <boost/optional.hpp>
 
 namespace ripple {
 
@@ -45,7 +46,7 @@ public:
     ~STParsedJSONObject () = default;
 
     /** The STObject if the parse was successful. */
-    std::unique_ptr <STObject> object;
+    boost::optional <STObject> object;
 
     /** On failure, an appropriate set of error values. */
     Json::Value error;
@@ -72,7 +73,7 @@ public:
     ~STParsedJSONArray () = default;
 
     /** The STArray if the parse was successful. */
-    std::unique_ptr <STArray> array;
+    boost::optional <STArray> array;
 
     /** On failure, an appropriate set of error values. */
     Json::Value error;

src/ripple/protocol/STTx.h

@@ -53,8 +53,7 @@ public:
     explicit STTx (SerialIter& sit);
     explicit STTx (TxType type);
 
-    // Only called from ripple::RPC::transactionSign - can we eliminate this?
+    explicit STTx (STObject&& object);
-    explicit STTx (STObject const& object);
 
     STBase*
     copy (std::size_t n, void* buf) const override

src/ripple/protocol/impl/STObject.cpp

@@ -71,16 +71,6 @@ STObject::STObject (SOTemplate const& type,
     setType (type);
 }
 
-STObject::STObject (SField const& name,
-        boost::ptr_vector<STBase>& data)
-    : STBase (name)
-    , mType (nullptr)
-{
-    v_.reserve(data.size());
-    for (auto const& b : data)
-        v_.emplace_back(b);
-}
-
 STObject::STObject (SerialIter& sit, SField const& name)
     : STBase(name)
     , mType(nullptr)

src/ripple/protocol/impl/STParsedJSON.cpp

@@ -31,6 +31,7 @@
 #include <ripple/protocol/STParsedJSON.h>
 #include <ripple/protocol/STPathSet.h>
 #include <ripple/protocol/TxFormats.h>
+#include <ripple/protocol/impl/STVar.h>
 #include <beast/module/core/text/LexicalCast.h>
 #include <cassert>
 #include <beast/cxx14/memory.h> // <memory>
@@ -140,14 +141,14 @@ static Json::Value singleton_expected (std::string const& object,
 
 // This function is used by parseObject to parse any JSON type that doesn't
 // recurse.  Everything represented here is a leaf-type.
-static std::unique_ptr <STBase> parseLeaf (
+static boost::optional<detail::STVar> parseLeaf (
     std::string const& json_name,
     std::string const& fieldName,
     SField const* name,
     Json::Value const& value,
     Json::Value& error)
 {
-    std::unique_ptr <STBase> ret;
+    boost::optional <detail::STVar> ret;
 
     auto const& field = SField::getField (fieldName);
 
@@ -174,7 +175,7 @@ static std::unique_ptr <STBase> parseLeaf (
                     return ret;
                 }
 
-                ret = std::make_unique <STUInt8> (field,
+                ret = detail::make_stvar <STUInt8> (field,
                     range_check_cast <unsigned char> (
                         value.asInt (), 0, 255));
             }
@@ -186,7 +187,7 @@ static std::unique_ptr <STBase> parseLeaf (
                     return ret;
                 }
 
-                ret = std::make_unique <STUInt8> (field,
+                ret = detail::make_stvar <STUInt8> (field,
                     range_check_cast <unsigned char> (
                         value.asUInt (), 0, 255));
             }
@@ -218,7 +219,7 @@ static std::unique_ptr <STBase> parseLeaf (
                         TxType const txType (TxFormats::getInstance().
                             findTypeByName (strValue));
 
-                        ret = std::make_unique <STUInt16> (field,
+                        ret = detail::make_stvar <STUInt16> (field,
                             static_cast <std::uint16_t> (txType));
 
                         if (*name == sfGeneric)
@@ -230,7 +231,7 @@ static std::unique_ptr <STBase> parseLeaf (
                             LedgerFormats::getInstance().
                                 findTypeByName (strValue));
 
-                        ret = std::make_unique <STUInt16> (field,
+                        ret = detail::make_stvar <STUInt16> (field,
                             static_cast <std::uint16_t> (type));
 
                         if (*name == sfGeneric)
@@ -244,19 +245,19 @@ static std::unique_ptr <STBase> parseLeaf (
                 }
                 else
                 {
-                    ret = std::make_unique <STUInt16> (field,
+                    ret = detail::make_stvar <STUInt16> (field,
                         beast::lexicalCastThrow <std::uint16_t> (strValue));
                 }
             }
             else if (value.isInt ())
             {
-                ret = std::make_unique <STUInt16> (field,
+                ret = detail::make_stvar <STUInt16> (field,
                     range_check_cast <std::uint16_t> (
                         value.asInt (), 0, 65535));
             }
             else if (value.isUInt ())
             {
-                ret = std::make_unique <STUInt16> (field,
+                ret = detail::make_stvar <STUInt16> (field,
                     range_check_cast <std::uint16_t> (
                         value.asUInt (), 0, 65535));
             }
@@ -279,19 +280,19 @@ static std::unique_ptr <STBase> parseLeaf (
         {
             if (value.isString ())
             {
-                ret = std::make_unique <STUInt32> (field,
+                ret = detail::make_stvar <STUInt32> (field,
                     beast::lexicalCastThrow <std::uint32_t> (
                         value.asString ()));
             }
             else if (value.isInt ())
             {
-                ret = std::make_unique <STUInt32> (field,
+                ret = detail::make_stvar <STUInt32> (field,
                     range_check_cast <std::uint32_t> (
                         value.asInt (), 0u, 4294967295u));
             }
             else if (value.isUInt ())
             {
-                ret = std::make_unique <STUInt32> (field,
+                ret = detail::make_stvar <STUInt32> (field,
                     static_cast <std::uint32_t> (value.asUInt ()));
             }
             else
@@ -313,18 +314,18 @@ static std::unique_ptr <STBase> parseLeaf (
         {
             if (value.isString ())
             {
-                ret = std::make_unique <STUInt64> (field,
+                ret = detail::make_stvar <STUInt64> (field,
                     uintFromHex (value.asString ()));
             }
             else if (value.isInt ())
             {
-                ret = std::make_unique <STUInt64> (field,
+                ret = detail::make_stvar <STUInt64> (field,
                     range_check_cast<std::uint64_t> (
                         value.asInt (), 0, 18446744073709551615ull));
             }
             else if (value.isUInt ())
             {
-                ret = std::make_unique <STUInt64> (field,
+                ret = detail::make_stvar <STUInt64> (field,
                     static_cast <std::uint64_t> (value.asUInt ()));
             }
             else
@@ -346,7 +347,7 @@ static std::unique_ptr <STBase> parseLeaf (
         {
             if (value.isString ())
             {
-                ret = std::make_unique <STHash128> (field, value.asString ());
+                ret = detail::make_stvar <STHash128> (field, value.asString ());
             }
             else
             {
@@ -367,7 +368,7 @@ static std::unique_ptr <STBase> parseLeaf (
         {
             if (value.isString ())
             {
-                ret = std::make_unique <STHash160> (field, value.asString ());
+                ret = detail::make_stvar <STHash160> (field, value.asString ());
             }
             else
             {
@@ -388,7 +389,7 @@ static std::unique_ptr <STBase> parseLeaf (
         {
             if (value.isString ())
             {
-                ret = std::make_unique <STHash256> (field, value.asString ());
+                ret = detail::make_stvar <STHash256> (field, value.asString ());
             }
             else
             {
@@ -418,7 +419,7 @@ static std::unique_ptr <STBase> parseLeaf (
             if (!vBlob.second)
                 throw std::invalid_argument ("invalid data");
 
-            ret = std::make_unique <STBlob> (field, vBlob.first.data (),
+            ret = detail::make_stvar <STBlob> (field, vBlob.first.data (),
                                              vBlob.first.size ());
         }
         catch (...)
@@ -432,7 +433,7 @@ static std::unique_ptr <STBase> parseLeaf (
     case STI_AMOUNT:
         try
         {
-            ret = std::make_unique <STAmount> (amountFromJson (field, value));
+            ret = detail::make_stvar <STAmount> (amountFromJson (field, value));
         }
         catch (...)
         {
@@ -451,15 +452,14 @@ static std::unique_ptr <STBase> parseLeaf (
 
         try
         {
-            std::unique_ptr <STVector256> tail =
+            STVector256 tail (field);
-                std::make_unique <STVector256> (field);
             for (Json::UInt i = 0; value.isValidIndex (i); ++i)
             {
                 uint256 s;
                 s.SetHex (value[i].asString ());
-                tail->push_back (s);
+                tail.push_back (s);
             }
-            ret = std::move (tail);
+            ret = detail::make_stvar <STVector256> (std::move (tail));
         }
         catch (...)
         {
@@ -478,8 +478,7 @@ static std::unique_ptr <STBase> parseLeaf (
 
         try
         {
-            std::unique_ptr <STPathSet> tail =
+            STPathSet tail (field);
-                std::make_unique <STPathSet> (field);
 
             for (Json::UInt i = 0; value.isValidIndex (i); ++i)
             {
@@ -597,9 +596,9 @@ static std::unique_ptr <STBase> parseLeaf (
                     p.emplace_back (uAccount, uCurrency, uIssuer, hasCurrency);
                 }
 
-                tail->push_back (p);
+                tail.push_back (p);
             }
-            ret = std::move (tail);
+            ret = detail::make_stvar <STPathSet> (std::move (tail));
         }
         catch (...)
         {
@@ -625,7 +624,7 @@ static std::unique_ptr <STBase> parseLeaf (
                 {
                     Account account;
                     account.SetHex (strValue);
-                    ret = std::make_unique <STAccount> (field, account);
+                    ret = detail::make_stvar <STAccount> (field, account);
                 }
                 else
                 {
@@ -639,7 +638,7 @@ static std::unique_ptr <STBase> parseLeaf (
                     }
 
                     ret =
-                        std::make_unique <STAccount> (field, a.getAccountID ());
+                        detail::make_stvar <STAccount> (field, a.getAccountID ());
                 }
             }
             catch (...)
@@ -661,42 +660,37 @@ static std::unique_ptr <STBase> parseLeaf (
 static const int maxDepth = 64;
 
 // Forward declaration since parseObject() and parseArray() call each other.
-static bool parseArray (
+static boost::optional <detail::STVar> parseArray (
     std::string const& json_name,
     Json::Value const& json,
     SField const& inName,
     int depth,
-    std::unique_ptr <STArray>& sub_array,
     Json::Value& error);
 
 
 
-static bool parseObject (
+static boost::optional <STObject> parseObject (
     std::string const& json_name,
     Json::Value const& json,
     SField const& inName,
     int depth,
-    std::unique_ptr <STObject>& sub_object,
     Json::Value& error)
 {
     if (! json.isObject ())
     {
         error = not_an_object (json_name);
-        return false;
+        return boost::none;
     }
 
     if (depth > maxDepth)
     {
         error = too_deep (json_name);
-        return false;
+        return boost::none;
     }
 
-    auto name (&inName);
+    STObject data (inName);
 
-    boost::ptr_vector<STBase> data;
+    for (auto const& fieldName : json.getMemberNames ())
-    Json::Value::Members members (json.getMemberNames ());
-
-    for (auto const& fieldName : members)
     {
         Json::Value const& value = json [fieldName];
 
@@ -705,7 +699,7 @@ static bool parseObject (
         if (field == sfInvalid)
         {
             error = unknown_field (json_name, fieldName);
-            return false;
+            return boost::none;
         }
 
         switch (field.fieldType)
@@ -719,22 +713,21 @@ static bool parseObject (
             if (! value.isObject ())
             {
                 error = not_an_object (json_name, fieldName);
-                return false;
+                return boost::none;
             }
 
             try
             {
-                std::unique_ptr <STObject> sub_object_;
+                auto ret = parseObject (json_name + "." + fieldName,
-                bool const success (parseObject (json_name + "." + fieldName,
+                    value, field, depth + 1, error);
-                    value, field, depth + 1, sub_object_, error));
+                if (! ret)
-                if (! success)
+                    return boost::none;
-                    return false;
+                data.emplace_back (std::move (*ret));
-                data.push_back (sub_object_.release ());
             }
             catch (...)
             {
                 error = invalid_data (json_name, fieldName);
-                return false;
+                return boost::none;
             }
 
             break;
@@ -743,17 +736,16 @@ static bool parseObject (
         case STI_ARRAY:
             try
             {
-                std::unique_ptr <STArray> sub_array_;
+                auto array = parseArray (json_name + "." + fieldName,
-                bool const success (parseArray (json_name + "." + fieldName,
+                    value, field, depth + 1, error);
-                    value, field, depth + 1, sub_array_, error));
+                if (array == boost::none)
-                if (! success)
+                    return boost::none;
-                    return false;
+                data.emplace_back (std::move (*array));
-                data.push_back (sub_array_.release ());
             }
             catch (...)
             {
                 error = invalid_data (json_name, fieldName);
-                return false;
+                return boost::none;
             }
 
             break;
@@ -761,46 +753,44 @@ static bool parseObject (
         // Everything else (types that don't recurse).
         default:
             {
-                std::unique_ptr <STBase> serTyp =
+                auto leaf =
-                    parseLeaf (json_name, fieldName, name, value, error);
+                    parseLeaf (json_name, fieldName, &inName, value, error);
 
-                if (!serTyp)
+                if (!leaf)
-                    return false;
+                    return boost::none;
 
-                data.push_back (serTyp.release ());
+                data.emplace_back (std::move (*leaf));
             }
 
             break;
         }
     }
 
-    sub_object = std::make_unique <STObject> (*name, data);
+    return std::move (data);
-    return true;
 }
 
-static bool parseArray (
+static boost::optional <detail::STVar> parseArray (
     std::string const& json_name,
     Json::Value const& json,
     SField const& inName,
     int depth,
-    std::unique_ptr <STArray>& sub_array,
     Json::Value& error)
 {
     if (! json.isArray ())
     {
         error = not_an_array (json_name);
-        return false;
+        return boost::none;
     }
 
     if (depth > maxDepth)
     {
         error = too_deep (json_name);
-        return false;
+        return boost::none;
     }
 
     try
     {
-        std::unique_ptr <STArray> tail = std::make_unique <STArray> (inName);
+        STArray tail (inName);
 
         for (Json::UInt i = 0; json.isValidIndex (i); ++i)
         {
@@ -810,7 +800,7 @@ static bool parseArray (
             if (!isObject || !singleKey)
             {
                 error = singleton_expected (json_name, i);
-                return false;
+                return boost::none;
             }
 
             // TODO: There doesn't seem to be a nice way to get just the
@@ -822,35 +812,34 @@ static bool parseArray (
             if (nameField == sfInvalid)
             {
                 error = unknown_field (json_name, objectName);
-                return false;
+                return boost::none;
             }
 
             Json::Value const objectFields (json[i][objectName]);
 
-            std::unique_ptr <STObject> sub_object_;
+            std::stringstream ss;
-            {
+            ss << json_name << "." <<
-                std::stringstream ss;
+                "[" << i << "]." << objectName;
-                ss << json_name << "." <<
-                    "[" << i << "]." << objectName;
-                bool const success (parseObject (ss.str (), objectFields,
-                    nameField, depth + 1, sub_object_, error));
 
-                if (! success ||
+            auto ret = parseObject (ss.str (), objectFields,
-                    (sub_object_->getFName().fieldType != STI_OBJECT))
+                nameField, depth + 1, error);
-                {
+
-                    return false;
+            if (! ret ||
-                }
+                (ret->getFName().fieldType != STI_OBJECT))
-            }
+	    {
-            tail->push_back (*sub_object_);
+	        return boost::none;
+	    }
+
+            tail.push_back (std::move (*ret));
         }
-        sub_array = std::move (tail);
+
+        return detail::make_stvar <STArray> (std::move (tail));
     }
     catch (...)
     {
         error = invalid_data (json_name);
-        return false;
+        return boost::none;
     }
-    return true;
 }
 
 } // STParsedJSONDetail
@@ -862,7 +851,7 @@ STParsedJSONObject::STParsedJSONObject (
     Json::Value const& json)
 {
     using namespace STParsedJSONDetail;
-    parseObject (name, json, sfGeneric, 0, object, error);
+    object = std::move (parseObject (name, json, sfGeneric, 0, error));
 }
 
 //------------------------------------------------------------------------------
@@ -872,7 +861,17 @@ STParsedJSONArray::STParsedJSONArray (
     Json::Value const& json)
 {
     using namespace STParsedJSONDetail;
-    parseArray (name, json, sfGeneric, 0, array, error);
+    auto arr = parseArray (name, json, sfGeneric, 0, error);
+    if (!arr)
+        array = boost::none;
+    else
+    {
+        auto p = dynamic_cast <STArray*> (&arr->get());
+        if (p == nullptr)
+            array = boost::none;
+        else
+            array = std::move (*p);
+    }
 }
 
 

src/ripple/protocol/impl/STTx.cpp

@@ -25,7 +25,6 @@
 #include <ripple/protocol/STAccount.h>
 #include <ripple/protocol/STArray.h>
 #include <ripple/protocol/STTx.h>
-#include <ripple/protocol/STParsedJSON.h>
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/basics/StringUtilities.h>
 #include <ripple/json/to_string.h>
@@ -54,8 +53,8 @@ STTx::STTx (TxType type)
     setFieldU16 (sfTransactionType, format->getType ());
 }
 
-STTx::STTx (STObject const& object)
+STTx::STTx (STObject&& object)
-    : STObject (object)
+    : STObject (std::move (object))
     , sig_state_ (boost::indeterminate)
 {
     tx_type_ = static_cast <TxType> (getFieldU16 (sfTransactionType));

src/ripple/protocol/impl/STVar.h

@@ -88,7 +88,14 @@ public:
     STBase const& operator*() const { return get(); }
     STBase const* operator->() const { return &get(); }
 
+    template <class T, class... Args>
+    friend
+    STVar
+    make_stvar(Args&&... args);
+
 private:
+    STVar() = default;
+
     STVar (SerializedTypeID id, SField const& name);
 
     void destroy();
@@ -113,6 +120,16 @@ private:
     }
 };
 
+template <class T, class... Args>
+inline
+STVar
+make_stvar(Args&&... args)
+{
+    STVar st;
+    st.construct<T>(std::forward<Args>(args)...);
+    return std::move(st);
+}
+
 inline
 bool
 operator== (STVar const& lhs, STVar const& rhs)

src/ripple/protocol/tests/STObject.test.cpp

@@ -75,7 +75,7 @@ public:
             bool parsedOK (parseJSONString(faulty, faultyJson));
             unexpected(!parsedOK, "failed to parse");
             STParsedJSONObject parsed ("test", faultyJson);
-            expect (parsed.object.get() == nullptr,
+            expect (! parsed.object,
                 "It should have thrown. "
                   "Immediate children of STArray encoded as json must "
                   "have one key only.");

src/ripple/protocol/tests/STTx.test.cpp

@@ -61,15 +61,13 @@ public:
         }
 
         STParsedJSONObject parsed ("test", j.getJson (0));
-        std::unique_ptr <STObject> new_obj (std::move (parsed.object));
+        if (!parsed.object)
-
-        if (new_obj.get () == nullptr)
             fail ("Unable to build object from json");
 
-        if (STObject (j) != *new_obj)
+        if (STObject (j) != parsed.object)
         {
             log << "ORIG: " << j.getJson (0);
-            log << "BUILT " << new_obj->getJson (0);
+            log << "BUILT " << parsed.object->getJson (0);
             fail ("Built a different transaction");
         }
         else

src/ripple/rpc/handlers/RipplePathFind.cpp

@@ -249,11 +249,11 @@ ripplePathFind(RippleLineCache::pointer const& cache,
             Json::Value pathSet = Json::objectValue;
             pathSet[jss::Paths] = contextPaths.get();
             STParsedJSONObject paths("pathSet", pathSet);
-            if (paths.object.get() == nullptr)
+            if (! paths.object)
                 return std::make_pair(false, paths.error);
             else
             {
-                spsComputed = paths.object.get()->getFieldPathSet(sfPaths);
+                spsComputed = paths.object->getFieldPathSet(sfPaths);
                 WriteLog(lsTRACE, RPCHandler) << "ripple_path_find: Paths: " <<
                     spsComputed.getJson(0);
             }

src/ripple/rpc/impl/TransactionSign.cpp

@@ -445,15 +445,14 @@ transactionSign (
     }
 
     STParsedJSONObject parsed (std::string (jss::tx_json), tx_json);
-    if (!parsed.object.get())
+    if (! parsed.object)
     {
         jvResult [jss::error] = parsed.error [jss::error];
         jvResult [jss::error_code] = parsed.error [jss::error_code];
         jvResult [jss::error_message] = parsed.error [jss::error_message];
         return jvResult;
     }
-    std::unique_ptr<STObject> sopTrans = std::move(parsed.object);
+    parsed.object->setFieldVL (
-    sopTrans->setFieldVL (
         sfSigningPubKey,
         keypair.publicKey.getAccountPublic());
 
@@ -461,7 +460,7 @@ transactionSign (
 
     try
     {
-        stpTrans = std::make_shared<STTx> (*sopTrans);
+        stpTrans = std::make_shared<STTx> (std::move (*parsed.object));
     }
     catch (std::exception&)
     {