diff --git a/CheatSheet.md b/CheatSheet.md index e157422eca..7ce9cc2d81 100644 --- a/CheatSheet.md +++ b/CheatSheet.md @@ -8,6 +8,8 @@ - Use long descriptive names instead of abbreviations. - Use "explicit" for single-argument ctors - Avoid globals especially objects with static storage duration +- Order class declarations as types, public, protected, private, then data. +- Prefer 'private' over 'protected' # Function diff --git a/src/cpp/ripple/ripple_HashRouter.cpp b/src/cpp/ripple/ripple_HashRouter.cpp index 1ad94f271a..22cbea8d91 100644 --- a/src/cpp/ripple/ripple_HashRouter.cpp +++ b/src/cpp/ripple/ripple_HashRouter.cpp @@ -7,6 +7,63 @@ // VFALCO TODO Inline the function definitions class HashRouter : public IHashRouter { +private: + /** An entry in the routing table. + */ + class Entry : public CountedObject + { + public: + Entry () + : mFlags (0) + { + } + + std::set const& peekPeers () const + { + return mPeers; + } + + void addPeer (uint64 peer) + { + if (peer != 0) + mPeers.insert (peer); + } + + bool hasPeer (uint64 peer) const + { + return mPeers.count (peer) > 0; + } + + int getFlags (void) const + { + return mFlags; + } + + bool hasFlag (int mask) const + { + return (mFlags & mask) != 0; + } + + void setFlag (int flagsToSet) + { + mFlags |= flagsToSet; + } + + void clearFlag (int flagsToClear) + { + mFlags &= ~flagsToClear; + } + + void swapSet (std::set & other) + { + mPeers.swap (other); + } + + private: + int mFlags; + std::set mPeers; + }; + public: explicit HashRouter (int holdTime) : mHoldTime (holdTime) @@ -21,27 +78,29 @@ public: bool setFlag (uint256 const& index, int flag); int getFlags (uint256 const& index); - HashRouterEntry getEntry (uint256 const& ); - bool swapSet (uint256 const& index, std::set& peers, int flag); private: + Entry getEntry (uint256 const& ); + + Entry& findCreateEntry (uint256 const& , bool& created); + boost::mutex mSuppressionMutex; // Stores all suppressed hashes and their expiration time - boost::unordered_map mSuppressionMap; + boost::unordered_map mSuppressionMap; // Stores all expiration times and the hashes indexed for them std::map< int, std::list > mSuppressionTimes; int mHoldTime; - - HashRouterEntry& findCreateEntry (uint256 const& , bool& created); }; -HashRouterEntry& HashRouter::findCreateEntry (uint256 const& index, bool& created) +//------------------------------------------------------------------------------ + +HashRouter::Entry& HashRouter::findCreateEntry (uint256 const& index, bool& created) { - boost::unordered_map::iterator fit = mSuppressionMap.find (index); + boost::unordered_map::iterator fit = mSuppressionMap.find (index); if (fit != mSuppressionMap.end ()) { @@ -65,7 +124,7 @@ HashRouterEntry& HashRouter::findCreateEntry (uint256 const& index, bool& create } mSuppressionTimes[now].push_back (index); - return mSuppressionMap.emplace (index, HashRouterEntry ()).first->second; + return mSuppressionMap.emplace (index, Entry ()).first->second; } bool HashRouter::addSuppression (uint256 const& index) @@ -77,7 +136,7 @@ bool HashRouter::addSuppression (uint256 const& index) return created; } -HashRouterEntry HashRouter::getEntry (uint256 const& index) +HashRouter::Entry HashRouter::getEntry (uint256 const& index) { boost::mutex::scoped_lock sl (mSuppressionMutex); @@ -99,7 +158,7 @@ bool HashRouter::addSuppressionPeer (uint256 const& index, uint64 peer, int& fla boost::mutex::scoped_lock sl (mSuppressionMutex); bool created; - HashRouterEntry& s = findCreateEntry (index, created); + Entry& s = findCreateEntry (index, created); s.addPeer (peer); flags = s.getFlags (); return created; @@ -124,13 +183,17 @@ bool HashRouter::addSuppressionFlags (uint256 const& index, int flag) bool HashRouter::setFlag (uint256 const& index, int flag) { + // VFALCO NOTE Comments like this belong in the HEADER file, + // and more importantly in a Javadoc comment so + // they appear in the generated documentation. + // // return: true = changed, false = unchanged assert (flag != 0); boost::mutex::scoped_lock sl (mSuppressionMutex); bool created; - HashRouterEntry& s = findCreateEntry (index, created); + Entry& s = findCreateEntry (index, created); if ((s.getFlags () & flag) == flag) return false; @@ -144,7 +207,7 @@ bool HashRouter::swapSet (uint256 const& index, std::set& peers, int fla boost::mutex::scoped_lock sl (mSuppressionMutex); bool created; - HashRouterEntry& s = findCreateEntry (index, created); + Entry& s = findCreateEntry (index, created); if ((s.getFlags () & flag) == flag) return false; diff --git a/src/cpp/ripple/ripple_IHashRouter.h b/src/cpp/ripple/ripple_IHashRouter.h index a02ad46ae6..55b1cd8af0 100644 --- a/src/cpp/ripple/ripple_IHashRouter.h +++ b/src/cpp/ripple/ripple_IHashRouter.h @@ -4,66 +4,25 @@ */ //============================================================================== -#ifndef RIPPLE_HASHROUTER_H -#define RIPPLE_HASHROUTER_H +#ifndef RIPPLE_HASHROUTER_RIPPLEHEADER +#define RIPPLE_HASHROUTER_RIPPLEHEADER +// VFALCO NOTE Are these the flags?? Why aren't we using a packed struct? // VFALCO TODO convert these macros to int constants #define SF_RELAYED 0x01 // Has already been relayed to other nodes +// VFALCO NOTE How can both bad and good be set on a hash? #define SF_BAD 0x02 // Signature/format is bad #define SF_SIGGOOD 0x04 // Signature is good #define SF_SAVED 0x08 #define SF_RETRY 0x10 // Transaction can be retried #define SF_TRUSTED 0x20 // comes from trusted source -// VFALCO TODO move this class into the scope of class HashRouter -class HashRouterEntry - : public CountedObject -{ -public: - HashRouterEntry () : mFlags (0) - { - ; - } - - const std::set& peekPeers () - { - return mPeers; - } - void addPeer (uint64 peer) - { - if (peer != 0) mPeers.insert (peer); - } - bool hasPeer (uint64 peer) - { - return mPeers.count (peer) > 0; - } - - int getFlags (void) - { - return mFlags; - } - bool hasFlag (int f) - { - return (mFlags & f) != 0; - } - void setFlag (int f) - { - mFlags |= f; - } - void clearFlag (int f) - { - mFlags &= ~f; - } - void swapSet (std::set& s) - { - mPeers.swap (s); - } - -protected: - int mFlags; - std::set mPeers; -}; +/** Routing table for objects identified by hash. + This table keeps track of which hashes have been received by which peers. + It is used to manage the routing and broadcasting of messages in the peer + to peer overlay. +*/ class IHashRouter { public: @@ -80,17 +39,29 @@ public: virtual ~IHashRouter () { } + // VFALCO TODO Replace "Supression" terminology with something more semantically meaningful. virtual bool addSuppression (uint256 const& index) = 0; virtual bool addSuppressionPeer (uint256 const& index, uint64 peer) = 0; + virtual bool addSuppressionPeer (uint256 const& index, uint64 peer, int& flags) = 0; + virtual bool addSuppressionFlags (uint256 const& index, int flag) = 0; - virtual bool setFlag (uint256 const& index, int flag) = 0; + + /** Set the flags on a hash. + + @return `true` if the flags were changed. + */ + // VFALCO TODO Rename to setFlags since it works with multiple flags. + virtual bool setFlag (uint256 const& index, int mask) = 0; + virtual int getFlags (uint256 const& index) = 0; - virtual HashRouterEntry getEntry (uint256 const& ) = 0; - virtual bool swapSet (uint256 const& index, std::set& peers, int flag) = 0; + + // VFALCO TODO This appears to be unused! + // +// virtual Entry getEntry (uint256 const&) = 0; };