Improve BuildInfo interface:

* Remove unnecessary beast::String dependency
* Explicitly cast to result type while packing a version
* Add unit tests for version formatting

src/ripple/data/protocol/BuildInfo.cpp

@@ -23,7 +23,9 @@
 
 namespace ripple {
 
-char const* BuildInfo::getRawVersionString ()
+namespace BuildInfo {
+
+char const* getRawVersionString ()
 {
     static char const* const rawText =
 
@@ -46,10 +48,9 @@ char const* BuildInfo::getRawVersionString ()
     return rawText;
 }
 
-BuildInfo::Protocol const& BuildInfo::getCurrentProtocol ()
+Protocol const& getCurrentProtocol ()
 {
     static Protocol currentProtocol (
-    
     //--------------------------------------------------------------------------
     //
     // The protocol version we speak and prefer (edit this if necessary)
@@ -63,7 +64,7 @@ BuildInfo::Protocol const& BuildInfo::getCurrentProtocol ()
     return currentProtocol;
 }
 
-BuildInfo::Protocol const& BuildInfo::getMinimumProtocol ()
+Protocol const& getMinimumProtocol ()
 {
     static Protocol minimumProtocol (
 
@@ -86,7 +87,7 @@ BuildInfo::Protocol const& BuildInfo::getMinimumProtocol ()
 //
 //------------------------------------------------------------------------------
 
-beast::String const& BuildInfo::getVersionString ()
+std::string const& getVersionString ()
 {
     struct SanityChecker
     {
@@ -102,67 +103,51 @@ beast::String const& BuildInfo::getVersionString ()
             versionString = rawText;
         }
 
-        beast::String versionString;
+        std::string versionString;
     };
 
-    static SanityChecker value;
+    static SanityChecker const value;
 
     return value.versionString;
 }
 
-char const* BuildInfo::getFullVersionString ()
+std::string const& getFullVersionString ()
 {
     struct PrettyPrinter
     {
         PrettyPrinter ()
         {
-            beast::String s;
-            
-            s << "rippled-" << getVersionString ();
-
-            fullVersionString = s.toStdString ();
+            fullVersionString = "rippled-" + getVersionString ();
         }
 
         std::string fullVersionString;
     };
 
-    static PrettyPrinter value;
+    static PrettyPrinter const value;
 
-    return value.fullVersionString.c_str ();
+    return value.fullVersionString;
 }
 
-//------------------------------------------------------------------------------
-
-BuildInfo::Protocol::Protocol ()
-    : vmajor (0)
-    , vminor (0)
+Protocol
+make_protocol (std::uint32_t version)
 {
+    return Protocol (
+        static_cast<std::uint16_t> ((version >> 16) & 0xffff),
+        static_cast<std::uint16_t> (version & 0xffff));
 }
 
-BuildInfo::Protocol::Protocol (unsigned short major_, unsigned short minor_)
-    : vmajor (major_)
-    , vminor (minor_)
-{
 }
 
-BuildInfo::Protocol::Protocol (PackedFormat packedVersion)
+std::string
+to_string (BuildInfo::Protocol const& p)
 {
-    vmajor = (packedVersion >> 16) & 0xffff;
-    vminor = (packedVersion & 0xffff);
+    return std::to_string (p.first) + "." + std::to_string (p.second);
 }
 
-BuildInfo::Protocol::PackedFormat BuildInfo::Protocol::toPacked () const noexcept
+std::uint32_t
+to_packed (BuildInfo::Protocol const& p)
 {
-    return ((vmajor << 16) & 0xffff0000) | (vminor & 0xffff);
-}
-
-std::string BuildInfo::Protocol::toStdString () const noexcept
-{
-    beast::String s;
-
-    s << beast::String (vmajor) << "." << beast::String (vminor);
-
-    return s.toStdString ();
+    return (static_cast<std::uint32_t> (p.first) << 16) + p.second;
 }
 
 //------------------------------------------------------------------------------
@@ -179,58 +164,79 @@ public:
         expect (v.parse (BuildInfo::getRawVersionString ()));
     }
 
-    void checkProtcol (unsigned short vmajor, unsigned short vminor)
+
+    BuildInfo::Protocol
+    from_version (std::uint16_t major, std::uint16_t minor)
     {
-        typedef BuildInfo::Protocol P;
-
-        expect (P (P (vmajor, vminor).toPacked ()) == P (vmajor, vminor));
-    }
-
-    void testProtocol ()
-    {
-        typedef BuildInfo::Protocol P;
-
-        testcase ("protocol");
-
-        expect (P (0, 0).toPacked () == 0);
-        expect (P (0, 1).toPacked () == 1);
-        expect (P (0, 65535).toPacked () == 65535);
-
-        checkProtcol (0, 0);
-        checkProtcol (0, 1);
-        checkProtcol (0, 255);
-        checkProtcol (0, 65535);
-        checkProtcol (1, 0);
-        checkProtcol (1, 65535);
-        checkProtcol (65535, 65535);
+        return BuildInfo::Protocol (major, minor);
     }
 
     void testValues ()
     {
         testcase ("comparison");
 
-        typedef BuildInfo::Protocol P;
+        expect (from_version (1,2) == from_version (1,2));
+        expect (from_version (3,4) >= from_version (3,4));
+        expect (from_version (5,6) <= from_version (5,6));
+        expect (from_version (7,8) >  from_version (6,7));
+        expect (from_version (7,8) <  from_version (8,9));
+        expect (from_version (65535,0) <  from_version (65535,65535));
+        expect (from_version (65535,65535) >= from_version (65535,65535));
+    }
 
-        expect (P(1,2) == P(1,2));
-        expect (P(3,4) >= P(3,4));
-        expect (P(5,6) <= P(5,6));
-        expect (P(7,8) >  P(6,7));
-        expect (P(7,8) <  P(8,9));
-        expect (P(65535,0) <  P(65535,65535));
-        expect (P(65535,65535) >= P(65535,65535));
+    void testStringVersion ()
+    {
+        testcase ("string version");
 
-        expect (BuildInfo::getCurrentProtocol () >= BuildInfo::getMinimumProtocol ());
+        for (std::uint16_t major = 0; major < 8; major++)
+        {
+            for (std::uint16_t minor = 0; minor < 8; minor++)
+            {
+                expect (to_string (from_version (major, minor)) ==
+                    std::to_string (major) + "." + std::to_string (minor));
+            }
+        }
+    }
+
+    void testVersionPacking ()
+    {
+        testcase ("version packing");
+
+        expect (to_packed (from_version (0, 0)) == 0);
+        expect (to_packed (from_version (0, 1)) == 1);
+        expect (to_packed (from_version (0, 255)) == 255);
+        expect (to_packed (from_version (0, 65535)) == 65535);
+
+        expect (to_packed (from_version (1, 0)) == 65536);
+        expect (to_packed (from_version (1, 1)) == 65537);
+        expect (to_packed (from_version (1, 255)) == 65791);
+        expect (to_packed (from_version (1, 65535)) == 131071);
+
+        expect (to_packed (from_version (255, 0)) == 16711680);
+        expect (to_packed (from_version (255, 1)) == 16711681);
+        expect (to_packed (from_version (255, 255)) == 16711935);
+        expect (to_packed (from_version (255, 65535)) == 16777215);
+
+        expect (to_packed (from_version (65535, 0)) == 4294901760);
+        expect (to_packed (from_version (65535, 1)) == 4294901761);
+        expect (to_packed (from_version (65535, 255)) == 4294902015);
+        expect (to_packed (from_version (65535, 65535)) == 4294967295);
     }
 
     void run ()
     {
         testVersion ();
-        testProtocol ();
         testValues ();
+        testStringVersion ();
+        testVersionPacking ();
 
-        log <<
-            "  Ripple version: " <<
-            BuildInfo::getVersionString().toStdString();
+        auto const current_protocol = BuildInfo::getCurrentProtocol ();
+        auto const minimum_protocol = BuildInfo::getMinimumProtocol ();
+
+        expect (current_protocol >= minimum_protocol);
+
+        log << "   Ripple Version: " << BuildInfo::getVersionString();
+        log << " Protocol Version: " << to_string (current_protocol);
     }
 };
 

src/ripple/data/protocol/BuildInfo.h

@@ -23,7 +23,7 @@
 namespace ripple {
 
 /** Versioning information for this build. */
-struct BuildInfo
+namespace BuildInfo
 {
     /** Server version.
 
@@ -31,14 +31,14 @@ struct BuildInfo
 
         http://semver.org/
     */
-    static beast::String const& getVersionString ();
+    std::string const& getVersionString ();
 
     /** Full server version string.
 
         This includes the name of the server. It is used in the peer
         protocol hello message and also the headers of some HTTP replies.
     */
-    static char const* getFullVersionString ();
+    std::string const& getFullVersionString ();
 
     //--------------------------------------------------------------------------
 
@@ -47,41 +47,27 @@ struct BuildInfo
         The version consists of two unsigned 16 bit integers representing
         major and minor version numbers. All values are permissible.
     */
-    struct Protocol
-    {
-        unsigned short vmajor;
-        unsigned short vminor;
+    using Protocol = std::pair <std::uint16_t const, std::uint16_t const>;
 
-        //----
-
-        /** The serialized format of the protocol version. */
-        typedef std::uint32_t PackedFormat;
-
-        Protocol ();
-        Protocol (unsigned short vmajor, unsigned short vminor);
-        explicit Protocol (PackedFormat packedVersion);
-
-        PackedFormat toPacked () const noexcept;
-
-        std::string toStdString () const noexcept;
-
-        bool operator== (Protocol const& other) const noexcept { return toPacked () == other.toPacked (); }
-        bool operator!= (Protocol const& other) const noexcept { return toPacked () != other.toPacked (); }
-        bool operator>= (Protocol const& other) const noexcept { return toPacked () >= other.toPacked (); }
-        bool operator<= (Protocol const& other) const noexcept { return toPacked () <= other.toPacked (); }
-        bool operator>  (Protocol const& other) const noexcept { return toPacked () >  other.toPacked (); }
-        bool operator<  (Protocol const& other) const noexcept { return toPacked () <  other.toPacked (); }
-    };
+    /** Construct a protocol version from a packed 32-bit protocol identifier */
+    Protocol
+    make_protocol (std::uint32_t version);
 
     /** The protocol version we speak and prefer. */
-    static Protocol const& getCurrentProtocol ();
+    Protocol const& getCurrentProtocol ();
 
     /** The oldest protocol version we will accept. */
-    static Protocol const& getMinimumProtocol ();
+    Protocol const& getMinimumProtocol ();
 
-    static char const* getRawVersionString ();
+    char const* getRawVersionString ();
 };
 
+std::string
+to_string (BuildInfo::Protocol const& p);
+
+std::uint32_t
+to_packed (BuildInfo::Protocol const& p);
+
 } // ripple
 
 #endif

src/ripple/overlay/impl/PeerImp.cpp

@@ -125,7 +125,7 @@ PeerImp::make_request()
     //m.headers.append ("Local-Address", m_socket->
     m.headers.append ("Remote-Address", m_remoteAddress.to_string());
     m.headers.append ("Upgrade",
-        std::string("Ripple/")+BuildInfo::getCurrentProtocol().toStdString());
+        std::string("Ripple/") + to_string (BuildInfo::getCurrentProtocol()));
     m.headers.append ("Connection", "Upgrade");
     m.headers.append ("Connect-As", "Leaf, Peer");
     m.headers.append ("Accept-Encoding", "identity, snappy");
@@ -585,7 +585,7 @@ PeerImp::on_message (std::shared_ptr <protocol::TMHello> const& m)
     }
 #endif
 
-    BuildInfo::Protocol protocol (m->protoversion());
+    auto protocol = BuildInfo::make_protocol(m->protoversion());
 
     if (m->has_nettime () &&
         ((m->nettime () < minTime) || (m->nettime () > maxTime)))
@@ -603,18 +603,12 @@ PeerImp::on_message (std::shared_ptr <protocol::TMHello> const& m)
                 " is off by -" << ourTime - m->nettime ();
         }
     }
-    else if (m->protoversionmin () > BuildInfo::getCurrentProtocol().toPacked ())
+    else if (m->protoversionmin () > to_packed (BuildInfo::getCurrentProtocol()))
     {
-        std::string reqVersion (
-            protocol.toStdString ());
-
-        std::string curVersion (
-            BuildInfo::getCurrentProtocol().toStdString ());
-
         m_journal.info <<
             "Hello: Disconnect: Protocol mismatch [" <<
-            "Peer expects " << reqVersion <<
-            " and we run " << curVersion << "]";
+            "Peer expects " << to_string (protocol) <<
+            " and we run " << to_string (BuildInfo::getCurrentProtocol()) << "]";
     }
     else if (! m_nodePublicKey.setNodePublic (m->nodepublic ()))
     {
@@ -638,7 +632,7 @@ PeerImp::on_message (std::shared_ptr <protocol::TMHello> const& m)
             m_journal.active(beast::Journal::Severity::kInfo))
         {
             m_journal.info <<
-                "Peer protocol: " << protocol.toStdString ();
+                "Peer protocol: " << to_string (protocol);
         }
 
         mHello = *m;

src/ripple/overlay/impl/PeerImp.h

@@ -554,10 +554,12 @@ public:
         if (mHello.has_fullversion ())
             ret["version"] = mHello.fullversion ();
 
-        if (mHello.has_protoversion () &&
-                (mHello.protoversion () != BuildInfo::getCurrentProtocol().toPacked ()))
+        if (mHello.has_protoversion ())
         {
-            ret["protocol"] = BuildInfo::Protocol (mHello.protoversion ()).toStdString ();
+            auto protocol = BuildInfo::make_protocol (mHello.protoversion ());
+
+            if (protocol != BuildInfo::getCurrentProtocol())
+                ret["protocol"] = to_string (protocol);
         }
 
         std::uint32_t minSeq, maxSeq;
@@ -876,8 +878,8 @@ private:
 
         protocol::TMHello h;
 
-        h.set_protoversion (BuildInfo::getCurrentProtocol().toPacked ());
-        h.set_protoversionmin (BuildInfo::getMinimumProtocol().toPacked ());
+        h.set_protoversion (to_packed (BuildInfo::getCurrentProtocol()));
+        h.set_protoversionmin (to_packed (BuildInfo::getMinimumProtocol()));
         h.set_fullversion (BuildInfo::getFullVersionString ());
         h.set_nettime (getApp().getOPs ().getNetworkTimeNC ());
         h.set_nodepublic (getApp().getLocalCredentials ().getNodePublic ().humanNodePublic ());

src/ripple/overlay/impl/handshake_analyzer.h

@@ -598,9 +598,9 @@ public:
             ret["version"] = mHello.fullversion ();
 
         if (mHello.has_protoversion () &&
-                (mHello.protoversion () != BuildInfo::getCurrentProtocol().toPacked ()))
+                (mHello.protoversion () != to_packed (BuildInfo::getCurrentProtocol())))
         {
-            ret["protocol"] = BuildInfo::Protocol (mHello.protoversion ()).toStdString ();
+            ret["protocol"] = to_string (BuildInfo::make_protocol (mHello.protoversion ()));
         }
 
         std::uint32_t minSeq, maxSeq;
@@ -960,8 +960,8 @@ private:
 
         protocol::TMHello h;
 
-        h.set_protoversion (BuildInfo::getCurrentProtocol().toPacked ());
-        h.set_protoversionmin (BuildInfo::getMinimumProtocol().toPacked ());
+        h.set_protoversion (to_packed (BuildInfo::getCurrentProtocol()));
+        h.set_protoversionmin (to_packed (BuildInfo::getMinimumProtocol()));
         h.set_fullversion (BuildInfo::getFullVersionString ());
         h.set_nettime (getApp().getOPs ().getNetworkTimeNC ());
         h.set_nodepublic (getApp().getLocalCredentials ().getNodePublic ().humanNodePublic ());