Refactor uses of strCopy (RIPD-1256)

Replace the sparsely used strCopy function with Slice. Change some of
the SHAMap interface to use Slice instead of Blob, which should
eliminate a copy.

src/ripple/app/ledger/InboundLedger.h

@@ -127,7 +127,7 @@ private:
     bool takeTxNode (const std::vector<SHAMapNodeID>& IDs,
                      const std::vector<Blob>& data,
                      SHAMapAddNode&);
-    bool takeTxRootNode (Blob const& data, SHAMapAddNode&);
+    bool takeTxRootNode (Slice const& data, SHAMapAddNode&);
 
     // VFALCO TODO Rename to receiveAccountStateNode
     //             Don't use acronyms, but if we are going to use them at least
@@ -136,7 +136,7 @@ private:
     bool takeAsNode (const std::vector<SHAMapNodeID>& IDs,
                      const std::vector<Blob>& data,
                      SHAMapAddNode&);
-    bool takeAsRootNode (Blob const& data, SHAMapAddNode&);
+    bool takeAsRootNode (Slice const& data, SHAMapAddNode&);
 
     std::vector<uint256>
     neededTxHashes (

src/ripple/app/ledger/impl/InboundLedger.cpp

@@ -856,14 +856,14 @@ bool InboundLedger::takeTxNode (const std::vector<SHAMapNodeID>& nodeIDs,
         if (nodeIDit->isRoot ())
         {
             san += mLedger->txMap().addRootNode (
-                SHAMapHash{mLedger->info().txHash}, *nodeDatait, snfWIRE, &tFilter);
+                SHAMapHash{mLedger->info().txHash}, makeSlice(*nodeDatait), snfWIRE, &tFilter);
             if (!san.isGood())
                 return false;
         }
         else
         {
             san +=  mLedger->txMap().addKnownNode (
-                *nodeIDit, *nodeDatait, &tFilter);
+                *nodeIDit, makeSlice(*nodeDatait), &tFilter);
             if (!san.isGood())
                 return false;
         }
@@ -926,7 +926,7 @@ bool InboundLedger::takeAsNode (const std::vector<SHAMapNodeID>& nodeIDs,
         if (nodeIDit->isRoot ())
         {
             san += mLedger->stateMap().addRootNode (
-                SHAMapHash{mLedger->info().accountHash}, *nodeDatait, snfWIRE, &tFilter);
+                SHAMapHash{mLedger->info().accountHash}, makeSlice(*nodeDatait), snfWIRE, &tFilter);
             if (!san.isGood ())
             {
                 JLOG (m_journal.warn()) <<
@@ -937,7 +937,7 @@ bool InboundLedger::takeAsNode (const std::vector<SHAMapNodeID>& nodeIDs,
         else
         {
             san += mLedger->stateMap().addKnownNode (
-                *nodeIDit, *nodeDatait, &tFilter);
+                *nodeIDit, makeSlice(*nodeDatait), &tFilter);
             if (!san.isGood ())
             {
                 JLOG (m_journal.warn()) <<
@@ -967,7 +967,7 @@ bool InboundLedger::takeAsNode (const std::vector<SHAMapNodeID>& nodeIDs,
 /** Process AS root node received from a peer
     Call with a lock
 */
-bool InboundLedger::takeAsRootNode (Blob const& data, SHAMapAddNode& san)
+bool InboundLedger::takeAsRootNode (Slice const& data, SHAMapAddNode& san)
 {
     if (mFailed || mHaveState)
     {
@@ -990,7 +990,7 @@ bool InboundLedger::takeAsRootNode (Blob const& data, SHAMapAddNode& san)
 /** Process AS root node received from a peer
     Call with a lock
 */
-bool InboundLedger::takeTxRootNode (Blob const& data, SHAMapAddNode& san)
+bool InboundLedger::takeTxRootNode (Slice const& data, SHAMapAddNode& san)
 {
     if (mFailed || mHaveTransactions)
     {
@@ -1106,14 +1106,14 @@ int InboundLedger::processData (std::shared_ptr<Peer> peer,
 
 
         if (!mHaveState && (packet.nodes ().size () > 1) &&
-            !takeAsRootNode (strCopy (packet.nodes (1).nodedata ()), san))
+            !takeAsRootNode (makeSlice(packet.nodes(1).nodedata ()), san))
         {
             JLOG (m_journal.warn()) <<
                 "Included AS root invalid";
         }
 
         if (!mHaveTransactions && (packet.nodes ().size () > 2) &&
-            !takeTxRootNode (strCopy (packet.nodes (2).nodedata ()), san))
+            !takeTxRootNode (makeSlice(packet.nodes(2).nodedata ()), san))
         {
             JLOG (m_journal.warn()) <<
                 "Included TX root invalid";

src/ripple/app/ledger/impl/InboundLedgers.cpp

@@ -243,7 +243,7 @@ public:
 
                 auto id_string = node.nodeid();
                 auto newNode = SHAMapAbstractNode::make(
-                    Blob (node.nodedata().begin(), node.nodedata().end()),
+                    makeSlice(node.nodedata()),
                     0, snfWIRE, SHAMapHash{uZero}, false, app_.journal ("SHAMapNodeID"),
                     SHAMapNodeID(id_string.data(), id_string.size()));
 

src/ripple/app/ledger/impl/TransactionAcquire.cpp

@@ -215,14 +215,14 @@ SHAMapAddNode TransactionAcquire::takeNodes (const std::list<SHAMapNodeID>& node
                 if (mHaveRoot)
                     JLOG (j_.debug()) << "Got root TXS node, already have it";
                 else if (!mMap->addRootNode (SHAMapHash{getHash ()},
-                                             *nodeDatait, snfWIRE, nullptr).isGood())
+                                             makeSlice(*nodeDatait), snfWIRE, nullptr).isGood())
                 {
                     JLOG (j_.warn()) << "TX acquire got bad root node";
                 }
                 else
                     mHaveRoot = true;
             }
-            else if (!mMap->addKnownNode (*nodeIDit, *nodeDatait, &sf).isGood())
+            else if (!mMap->addKnownNode (*nodeIDit, makeSlice(*nodeDatait), &sf).isGood())
             {
                 JLOG (j_.warn()) << "TX acquire got bad non-root node";
                 return SHAMapAddNode::invalid ();

src/ripple/basics/StringUtilities.h

@@ -79,15 +79,10 @@ inline static std::string sqlEscape (Blob const& vecSrc)
     return j;
 }
 
-int strUnHex (std::string& strDst, std::string const& strSrc);
-
 uint64_t uintFromHex (std::string const& strSrc);
 
 std::pair<Blob, bool> strUnHex (std::string const& strSrc);
 
-Blob strCopy (std::string const& strSrc);
-std::string strCopy (Blob const& vucSrc);
-
 bool parseUrl (std::string const& strUrl, std::string& strScheme,
                std::string& strDomain, int& iPort, std::string& strPath);
 

src/ripple/basics/impl/StringUtilities.cpp

@@ -19,6 +19,7 @@
 
 #include <BeastConfig.h>
 #include <ripple/basics/contract.h>
+#include <ripple/basics/Slice.h>
 #include <ripple/basics/StringUtilities.h>
 #include <ripple/basics/ToString.h>
 #include <ripple/beast/core/LexicalCast.h>
@@ -31,14 +32,11 @@
 
 namespace ripple {
 
-// NIKB NOTE: This function is only used by strUnHex (std::string const& strSrc)
+std::pair<Blob, bool> strUnHex (std::string const& strSrc)
-// which results in a pointless copy from std::string into std::vector. Should
-// we just scrap this function altogether?
-int strUnHex (std::string& strDst, std::string const& strSrc)
 {
-    std::string tmp;
+    Blob out;
 
-    tmp.reserve ((strSrc.size () + 1) / 2);
+    out.reserve ((strSrc.size () + 1) / 2);
 
     auto iter = strSrc.cbegin ();
 
@@ -47,9 +45,9 @@ int strUnHex (std::string& strDst, std::string const& strSrc)
         int c = charUnHex (*iter);
 
         if (c < 0)
-            return -1;
+            return std::make_pair (Blob (), false);
 
-        tmp.push_back(c);
+        out.push_back(c);
         ++iter;
     }
 
@@ -59,30 +57,18 @@ int strUnHex (std::string& strDst, std::string const& strSrc)
         ++iter;
 
         if (cHigh < 0)
-            return -1;
+            return std::make_pair (Blob (), false);
 
         int cLow = charUnHex (*iter);
         ++iter;
 
         if (cLow < 0)
-            return -1;
-
-        tmp.push_back (static_cast<char>((cHigh << 4) | cLow));
-    }
-
-    strDst = std::move(tmp);
-
-    return strDst.size ();
-}
-
-std::pair<Blob, bool> strUnHex (std::string const& strSrc)
-{
-    std::string strTmp;
-
-    if (strUnHex (strTmp, strSrc) == -1)
             return std::make_pair (Blob (), false);
 
-    return std::make_pair(strCopy (strTmp), true);
+        out.push_back (static_cast<unsigned char>((cHigh << 4) | cLow));
+    }
+
+    return std::make_pair(std::move(out), true);
 }
 
 uint64_t uintFromHex (std::string const& strSrc)
@@ -105,33 +91,6 @@ uint64_t uintFromHex (std::string const& strSrc)
     return uValue;
 }
 
-//
-// Misc string
-//
-
-Blob strCopy (std::string const& strSrc)
-{
-    Blob vucDst;
-
-    vucDst.resize (strSrc.size ());
-
-    std::copy (strSrc.begin (), strSrc.end (), vucDst.begin ());
-
-    return vucDst;
-}
-
-std::string strCopy (Blob const& vucSrc)
-{
-    std::string strDst;
-
-    strDst.resize (vucSrc.size ());
-
-    std::copy (vucSrc.begin (), vucSrc.end (), strDst.begin ());
-
-    return strDst;
-
-}
-
 // TODO Callers should be using beast::URL and beast::parse_URL instead.
 bool parseUrl (std::string const& strUrl, std::string& strScheme, std::string& strDomain, int& iPort, std::string& strPath)
 {

src/ripple/basics/tests/StringUtilities.test.cpp

@@ -18,6 +18,7 @@
 //==============================================================================
 
 #include <BeastConfig.h>
+#include <ripple/basics/Slice.h>
 #include <ripple/basics/StringUtilities.h>
 #include <ripple/basics/ToString.h>
 #include <ripple/beast/unit_test.h>
@@ -27,18 +28,18 @@ namespace ripple {
 class StringUtilities_test : public beast::unit_test::suite
 {
 public:
-    void testUnHexSuccess (std::string strIn, std::string strExpected)
+    void testUnHexSuccess (std::string const& strIn, std::string const& strExpected)
     {
-        std::string strOut;
+        auto rv = strUnHex (strIn);
-        BEAST_EXPECT(strUnHex (strOut, strIn) == strExpected.length ());
+        BEAST_EXPECT(rv.second);
-        BEAST_EXPECT(strOut == strExpected);
+        BEAST_EXPECT(makeSlice(rv.first) == makeSlice(strExpected));
     }
 
-    void testUnHexFailure (std::string strIn)
+    void testUnHexFailure (std::string const& strIn)
     {
-        std::string strOut;
+        auto rv = strUnHex (strIn);
-        BEAST_EXPECT(strUnHex (strOut, strIn) == -1);
+        BEAST_EXPECT(! rv.second);
-        BEAST_EXPECT(strOut.empty ());
+        BEAST_EXPECT(rv.first.empty());
     }
 
     void testUnHex ()

src/ripple/shamap/SHAMap.h

@@ -195,9 +195,9 @@ public:
 
     bool getRootNode (Serializer & s, SHANodeFormat format) const;
     std::vector<uint256> getNeededHashes (int max, SHAMapSyncFilter * filter);
-    SHAMapAddNode addRootNode (SHAMapHash const& hash, Blob const& rootNode,
+    SHAMapAddNode addRootNode (SHAMapHash const& hash, Slice const& rootNode,
                                SHANodeFormat format, SHAMapSyncFilter * filter);
-    SHAMapAddNode addKnownNode (SHAMapNodeID const& nodeID, Blob const& rawNode,
+    SHAMapAddNode addKnownNode (SHAMapNodeID const& nodeID, Slice const& rawNode,
                                 SHAMapSyncFilter * filter);
 
     // status functions

src/ripple/shamap/SHAMapTreeNode.h

@@ -132,7 +132,7 @@ public:
     virtual void invariants(bool is_v2, bool is_root = false) const = 0;
 
     static std::shared_ptr<SHAMapAbstractNode>
-        make(Blob const& rawNode, std::uint32_t seq, SHANodeFormat format,
+        make(Slice const& rawNode, std::uint32_t seq, SHANodeFormat format,
              SHAMapHash const& hash, bool hashValid, beast::Journal j,
              SHAMapNodeID const& id = SHAMapNodeID{});
 
@@ -181,7 +181,7 @@ public:
     void invariants(bool is_v2, bool is_root = false) const override;
 
     friend std::shared_ptr<SHAMapAbstractNode>
-        SHAMapAbstractNode::make(Blob const& rawNode, std::uint32_t seq,
+        SHAMapAbstractNode::make(Slice const& rawNode, std::uint32_t seq,
              SHANodeFormat format, SHAMapHash const& hash, bool hashValid,
                  beast::Journal j, SHAMapNodeID const& id);
 
@@ -218,7 +218,7 @@ public:
     void invariants(bool is_v2, bool is_root = false) const override;
 
     friend std::shared_ptr<SHAMapAbstractNode>
-        SHAMapAbstractNode::make(Blob const& rawNode, std::uint32_t seq,
+        SHAMapAbstractNode::make(Slice const& rawNode, std::uint32_t seq,
              SHANodeFormat format, SHAMapHash const& hash, bool hashValid,
                  beast::Journal j, SHAMapNodeID const& id);
 };

src/ripple/shamap/impl/SHAMap.cpp

@@ -252,7 +252,7 @@ SHAMap::fetchNodeFromDB (SHAMapHash const& hash) const
         {
             try
             {
-                node = SHAMapAbstractNode::make(obj->getData(),
+                node = SHAMapAbstractNode::make(makeSlice(obj->getData()),
                     0, snfPREFIX, hash, true, f_.journal());
                 if (node && node->isInner())
                 {
@@ -309,7 +309,7 @@ SHAMap::checkFilter(SHAMapHash const& hash,
     if (filter->haveNode (hash, nodeData))
     {
         node = SHAMapAbstractNode::make(
-            nodeData, 0, snfPREFIX, hash, true, f_.journal ());
+            makeSlice(nodeData), 0, snfPREFIX, hash, true, f_.journal ());
         if (node)
         {
             filter->gotNode (true, hash,
@@ -496,7 +496,7 @@ SHAMap::descendAsync (SHAMapInnerNode* parent, int branch,
             if (!obj)
                 return nullptr;
 
-            ptr = SHAMapAbstractNode::make(obj->getData(), 0, snfPREFIX,
+            ptr = SHAMapAbstractNode::make(makeSlice(obj->getData()), 0, snfPREFIX,
                                            hash, true, f_.journal());
             if (ptr && backed_)
                 canonicalize (hash, ptr);

src/ripple/shamap/impl/SHAMapSync.cpp

@@ -422,7 +422,7 @@ bool SHAMap::getRootNode (Serializer& s, SHANodeFormat format) const
     return true;
 }
 
-SHAMapAddNode SHAMap::addRootNode (SHAMapHash const& hash, Blob const& rootNode, SHANodeFormat format,
+SHAMapAddNode SHAMap::addRootNode (SHAMapHash const& hash, Slice const& rootNode, SHANodeFormat format,
                                    SHAMapSyncFilter* filter)
 {
     // we already have a root_ node
@@ -459,7 +459,7 @@ SHAMapAddNode SHAMap::addRootNode (SHAMapHash const& hash, Blob const& rootNode,
 }
 
 SHAMapAddNode
-SHAMap::addKnownNode (const SHAMapNodeID& node, Blob const& rawNode,
+SHAMap::addKnownNode (const SHAMapNodeID& node, Slice const& rawNode,
                       SHAMapSyncFilter* filter)
 {
     // return value: true=okay, false=error

src/ripple/shamap/impl/SHAMapTreeNode.cpp

@@ -98,7 +98,7 @@ SHAMapTreeNode::SHAMapTreeNode (std::shared_ptr<SHAMapItem const> const& item,
 }
 
 std::shared_ptr<SHAMapAbstractNode>
-SHAMapAbstractNode::make(Blob const& rawNode, std::uint32_t seq, SHANodeFormat format,
+SHAMapAbstractNode::make(Slice const& rawNode, std::uint32_t seq, SHANodeFormat format,
                          SHAMapHash const& hash, bool hashValid, beast::Journal j,
                          SHAMapNodeID const& id)
 {
@@ -108,7 +108,7 @@ SHAMapAbstractNode::make(Blob const& rawNode, std::uint32_t seq, SHANodeFormat f
             return {};
 
         Serializer s (rawNode.data(), rawNode.size() - 1);
-        int type = rawNode.back ();
+        int type = rawNode[rawNode.size() - 1];
         int len = s.getLength ();
 
         if ((type < 0) || (type > 6))
@@ -265,7 +265,7 @@ SHAMapAbstractNode::make(Blob const& rawNode, std::uint32_t seq, SHANodeFormat f
         if (prefix == HashPrefix::transactionID)
         {
             auto item = std::make_shared<SHAMapItem const>(
-                sha512Half(makeSlice(rawNode)),
+                sha512Half(rawNode),
                     s.peekData ());
             if (hashValid)
                 return std::make_shared<SHAMapTreeNode>(item, tnTRANSACTION_NM, seq, hash);

src/ripple/shamap/tests/SHAMapSync.test.cpp

@@ -147,7 +147,7 @@ public:
 
             BEAST_EXPECT(destination.addRootNode (
                 source.getHash(),
-                *gotNodes_a.begin (),
+                makeSlice(*gotNodes_a.begin ()),
                 snfWIRE,
                 nullptr).isGood());
         }
@@ -184,7 +184,7 @@ public:
                 BEAST_EXPECT(
                     destination.addKnownNode (
                         gotNodeIDs_b[i],
-                        gotNodes_b[i],
+                        makeSlice(gotNodes_b[i]),
                         nullptr).isUseful ());
             }
         }