Tidy up and annotate

RefactoringNotes.txt

@@ -0,0 +1,51 @@
+--------------------------------------------------------------------------------
+
+Naming
+
+Some names don't make sense.
+
+  LedgerAcquire
+    Not a noun.
+    Is it really an InboundLedger ?
+    Does it continue to exist after the ledger is received?
+
+Inconsistent names
+
+  We have full names like SerializedType and then acronyms like STObject
+  Two names for some things, e.g. SerializedLedgerEntry and SLE
+
+  Shared/Smart pointer typedefs in classes have a variety of different names
+  for the same thing. e.g. "pointer", "ptr", "ptr_t", "wptr"
+
+Verbose names
+
+  The prefix "Flat" is more appealing than "Serialized" because its shorter and
+    easier to pronounce.
+
+Ledger "Skip List"
+
+  Is not really a skip list
+
+--------------------------------------------------------------------------------
+
+Interfaces
+
+  Serializer
+
+    Upon analysis this class does two incompatible things. Flattening, and
+    unflattening. The interface should be reimplemented as two distinct
+    abstract classes, InputStream and OutputStream with suitable implementations
+    such as to and from a block of memory or dynamically allocated buffer.
+
+    The name and conflation of dual roles serves to confuse code at the point
+    of call. Does set(Serializer& s) flatten or unflatten the data? This
+    would be more clear:
+        bool write (OutputStream& stream);
+
+
+
+Implementation
+
+  LoadManager
+    
+    What is going on in the destructor

StyleCheatSheet.txt

@@ -3,3 +3,5 @@
 - Place each ctor-initializer on its own line.
 - Create typedefs for primitive types to describe them.
 - Return descriptive local variables instead of constants.
+- Use "explicit" for single-argument ctors
+

modules/ripple_data/protocol/ripple_FieldNames.h

@@ -23,6 +23,11 @@ enum SerializedTypeID
 	STI_VALIDATION	= 10003,
 };
 
+/** Identifies fields.
+
+    Fields are necessary to tag data in signed transactions so that
+    the binary format of the transaction can be canonicalized.
+*/
 // VFALCO TODO rename this to NamedField
 class SField
 {
@@ -48,24 +53,42 @@ public:
 	int						fieldNum;
 	bool					signingField;
 
-	SField(int fc, SerializedTypeID tid, int fv, const char* fn) : 
+	SField (int fc, SerializedTypeID tid, int fv, const char* fn)
-		fieldCode(fc), fieldType(tid), fieldValue(fv), fieldName(fn), fieldMeta(sMD_Default), signingField(true)
+        : fieldCode (fc)
+        , fieldType (tid)
+        , fieldValue (fv)
+        , fieldName (fn)
+        , fieldMeta (sMD_Default)
+        , signingField (true)
 	{
 		boost::mutex::scoped_lock sl(mapMutex);
+
         codeToField[fieldCode] = this;
+		
         fieldNum = ++num;
 	}
 
-	SField(SerializedTypeID tid, int fv, const char *fn) :
+	SField (SerializedTypeID tid, int fv, const char *fn)
-		fieldCode(FIELD_CODE(tid, fv)), fieldType(tid), fieldValue(fv), fieldName(fn),
+        : fieldCode (FIELD_CODE (tid, fv))
-		fieldMeta(sMD_Default), signingField(true)
+        , fieldType (tid)
+        , fieldValue (fv)
+        , fieldName (fn)
+        , fieldMeta (sMD_Default)
+        , signingField (true)
 	{
 		boost::mutex::scoped_lock sl(mapMutex);
+
         codeToField[fieldCode] = this;
+		
         fieldNum = ++num;
 	}
 
-	SField(int fc) : fieldCode(fc), fieldType(STI_UNKNOWN), fieldValue(0), fieldMeta(sMD_Never), signingField(true)
+	explicit SField (int fc)
+        : fieldCode (fc)
+        , fieldType (STI_UNKNOWN)
+        , fieldValue (0)
+        , fieldMeta (sMD_Never)
+        , signingField (true)
 	{
 		boost::mutex::scoped_lock sl(mapMutex);
 		fieldNum = ++num;
@@ -86,7 +109,10 @@ public:
 	bool isUseful() const		{ return fieldCode > 0; }
 	bool isKnown() const		{ return fieldType != STI_UNKNOWN; }
 	bool isBinary() const		{ return fieldValue < 256; }
+
+    // VFALCO NOTE What is a discardable field?
     bool isDiscardable() const	{ return fieldValue > 256; }
+
     int getCode() const			{ return fieldCode; }
 	int getNum() const			{ return fieldNum; }
 	static int getNumFields()	{ return num; }
@@ -100,6 +126,7 @@ public:
 		{ return (fieldValue < 256) && (withSigningField || signingField); }
 
 	bool operator== (const SField& f) const { return fieldCode == f.fieldCode; }
+
     bool operator!= (const SField& f) const { return fieldCode != f.fieldCode; }
 
 	static int compare(SField::ref f1, SField::ref f2);

modules/ripple_data/protocol/ripple_SerializeDeclarations.h

@@ -1,6 +1,15 @@
 // This is not really a header file, but it can be used as one with
 // appropriate #define statements.
 
+/*
+    Common type common field - 1 byte
+    Common type uncommon field - 2 bytes
+    ...
+
+    Rarity of fields determines the number of bytes.
+    This is done to reduce the average size of the messages.
+*/
+
 	// types (common)
 	TYPE(Int16,				UINT16,		1)
 	TYPE(Int32,				UINT32,		2)

modules/ripple_data/protocol/ripple_SerializedObject.cpp

@@ -62,6 +62,7 @@ UPTR_T<SerializedType> STObject::makeDefaultObject(SerializedTypeID id, SField::
 	}
 }
 
+// VFALCO TODO Remove the 'depth' parameter
 UPTR_T<SerializedType> STObject::makeDeserializedObject(SerializedTypeID id, SField::ref name,
 	SerializerIterator& sit, int depth)
 {
@@ -184,6 +185,7 @@ bool STObject::setType(const SOTemplate &type)
 bool STObject::isValidForType()
 {
 	boost::ptr_vector<SerializedType>::iterator it = mData.begin();
+	
     BOOST_FOREACH(const SOElement* elem, mType->peek())
 	{
 		if (it == mData.end())

modules/ripple_data/protocol/ripple_SerializedObject.h

@@ -38,8 +38,13 @@ public:
 	virtual bool isDefault() const { return mData.empty(); }
 
 	virtual void add(Serializer& s) const	{ add(s, true);	} // just inner elements
+
     void add(Serializer& s, bool withSignature) const;
+
+    // VFALCO NOTE does this return an expensive copy of an object with a dynamic buffer?
+    // VFALCO TODO Remove this function and fix the few callers.
 	Serializer getSerializer() const { Serializer s; add(s); return s; }
+
 	std::string getFullText() const;
 	std::string getText() const;
 	virtual Json::Value getJson(int options) const;
@@ -117,13 +122,23 @@ public:
 	void delField(int index);
 
 	static UPTR_T <SerializedType> makeDefaultObject(SerializedTypeID id, SField::ref name);
-	static UPTR_T<SerializedType> makeDeserializedObject(SerializedTypeID id, SField::ref name,
+
-		SerializerIterator&, int depth);
+    // VFALCO TODO remove the 'depth' parameter
+    static UPTR_T<SerializedType> makeDeserializedObject (
+        SerializedTypeID id,
+        SField::ref name,
+        SerializerIterator &,
+        int depth);
 
 	static UPTR_T<SerializedType> makeNonPresentObject (SField::ref name)
-	{ return makeDefaultObject(STI_NOTPRESENT, name); }
+	{
+        return makeDefaultObject(STI_NOTPRESENT, name);
+    }
+
     static UPTR_T<SerializedType> makeDefaultObject (SField::ref name)
-	{ return makeDefaultObject(name.fieldType, name); }
+	{
+        return makeDefaultObject(name.fieldType, name);
+    }
 
 	// field iterator stuff
 	typedef boost::ptr_vector<SerializedType>::iterator iterator;
@@ -148,9 +163,7 @@ private:
 	{ mData.swap(data); }
 };
 
-// allow ptr_* collections of STObject's
+//------------------------------------------------------------------------------
-inline STObject* new_clone(const STObject& s) { return s.oClone().release(); }
-inline void delete_clone(const STObject* s) { boost::checked_delete(s); }
 
 inline STObject::iterator range_begin(STObject& x)		{ return x.begin(); }
 inline STObject::iterator range_end(STObject &x)		{ return x.end(); }

modules/ripple_data/protocol/ripple_SerializedObjectTemplate.h

@@ -50,6 +50,9 @@ public:
     */
 	SOTemplate ();
 
+    // VFALCO NOTE Why do we even bother with the 'private' keyword if
+    //             this function is present?
+    //
     std::vector <SOElement const*> const& peek() const
     {
         return mTypes;
@@ -65,6 +68,7 @@ public:
 
 private:
 	std::vector <SOElement const*> mTypes;
+
     std::vector <int> mIndex;		// field num -> index
 };
 

newcoin.vcxproj

@@ -1838,6 +1838,7 @@
     <Reference Include="System.Xml" />
   </ItemGroup>
   <ItemGroup>
+    <Text Include="RefactoringNotes.txt" />
     <Text Include="StyleCheatSheet.txt" />
   </ItemGroup>
   <Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />

newcoin.vcxproj.filters

@@ -1606,5 +1606,6 @@
   </ItemGroup>
   <ItemGroup>
     <Text Include="StyleCheatSheet.txt" />
+    <Text Include="RefactoringNotes.txt" />
   </ItemGroup>
 </Project>

src/cpp/ripple/LedgerConsensus.h

@@ -8,6 +8,7 @@
 DEFINE_INSTANCE(LedgerConsensus);
 DEFINE_INSTANCE(TransactionAcquire);
 
+// VFALCO TODO rename to PeerTxRequest
 // A transaction set we are trying to acquire
 class TransactionAcquire
     : private IS_INSTANCE (TransactionAcquire)
@@ -18,7 +19,7 @@ public:
 	typedef boost::shared_ptr<TransactionAcquire> pointer;
 
 public:
-	TransactionAcquire(uint256 const& hash);
+	explicit TransactionAcquire(uint256 const& hash);
 	virtual ~TransactionAcquire()		{ ; }
 
 	SHAMap::ref getMap()				{ return mMap; }

src/cpp/ripple/LedgerMaster.cpp

@@ -132,6 +132,8 @@ Ledger::pointer LedgerMaster::closeLedger(bool recover)
 			}
 		}
 		CondLog (recovers != 0, lsINFO, LedgerMaster) << "Recovered " << recovers << " held transactions";
+
+        // VFALCO TODO recreate the CanonicalTxSet object instead of resetting it
         mHeldTransactions.reset(closingLedger->getHash());
 	}
 

src/cpp/ripple/LedgerMaster.h

@@ -14,10 +14,17 @@ public:
 	typedef FUNCTION_TYPE<void(Ledger::ref)> callback;
 
 public:
-	LedgerMaster() : mHeldTransactions(uint256()), mMissingSeq(0),
+	LedgerMaster ()
-		mMinValidations(0), mLastValidateSeq(0), mPubThread(false),
+        : mHeldTransactions (uint256())
-		mPathFindThread(false), mPathFindNewLedger(false), mPathFindNewRequest(false)
+        , mMissingSeq (0)
-	{ ; }
+        , mMinValidations (0)
+        , mLastValidateSeq (0)
+        , mPubThread (false)
+        , mPathFindThread (false)
+        , mPathFindNewLedger (false)
+        , mPathFindNewRequest (false)
+	{
+    }
 
 	uint32 getCurrentLedgerIndex();
 
@@ -133,6 +140,7 @@ private:
 	void pubThread();
 	void updatePaths();
 
+private:
     boost::recursive_mutex mLock;
 
 	TransactionEngine mEngine;

src/cpp/ripple/ripple_AcceptedLedger.h

@@ -3,6 +3,11 @@
 
 /**
 
+    An accepted ledger is a ledger that has a sufficient number of
+    validations to convince the local server that it is irrevocable.
+
+    The existence of an accepted ledger implies all preceding ledgers
+    are accepted.
 */
 class AcceptedLedger
 {

src/cpp/ripple/ripple_AcceptedLedgerTx.h

@@ -1,12 +1,20 @@
 #ifndef RIPPLE_ACCEPTEDLEDGERTX_H
 #define RIPPLE_ACCEPTEDLEDGERTX_H
 
-/*============================================================================*/
+//------------------------------------------------------------------------------
+
 /**
     A transaction that is in a closed ledger.
 
     Description
 
+    An accepted ledger transaction contains additional information that the
+    server needs to tell clients about the transaction. For example,
+        - The transaction in JSON form
+        - Which accounts are affected
+          * This is used by InfoSub to report to clients
+        - Cached stuff
+
 	@code
     @endcode
 

src/cpp/ripple/ripple_CanonicalTXSet.h

@@ -1,6 +1,14 @@
 #ifndef RIPPLE_CANONICALTXSET_H
 #define RIPPLE_CANONICALTXSET_H
 
+/** Holds transactions which were deferred to the next pass of consensus.
+
+    "Canonical" refers to the order in which transactions are applied.
+
+    - Puts transactions from the same account in sequence order
+
+*/
+// VFALCO TODO rename to SortedTxSet
 class CanonicalTXSet
 {
 public:
@@ -25,7 +33,8 @@ public:
 	    uint256 const& getTXID() const					{ return mTXid; }
 
     private:
-	    uint256 mAccount, mTXid;
+	    uint256 mAccount;
+        uint256 mTXid;
 	    uint32 mSeq;
     };
 
@@ -33,13 +42,14 @@ public:
 	typedef std::map <Key, SerializedTransaction::pointer>::const_iterator const_iterator;
 
 public:
-	CanonicalTXSet (LedgerHash const& lastClosedLedgerHash)
+	explicit CanonicalTXSet (LedgerHash const& lastClosedLedgerHash)
         : mSetHash (lastClosedLedgerHash)
     {
     }
 
 	void push_back (SerializedTransaction::ref txn);
 
+    // VFALCO TODO remove this function
 	void reset (LedgerHash const& newLastClosedLedgerHash)
 	{
 		mSetHash = newLastClosedLedgerHash;
@@ -57,6 +67,7 @@ public:
 	bool empty() const				{ return mMap.empty(); }
 
 private:
+    // Used to salt the accounts so people can't mine for low account numbers
 	uint256 mSetHash;
 
     std::map <Key, SerializedTransaction::pointer> mMap;

src/cpp/ripple/ripple_LedgerAcquire.h

@@ -3,7 +3,7 @@
 
 DEFINE_INSTANCE(LedgerAcquire);
 
-// VFALCO TODO Rename to IncomingLedger
+// VFALCO TODO Rename to InboundLedger
 // A ledger we are trying to acquire
 class LedgerAcquire : private IS_INSTANCE(LedgerAcquire)
                     , public PeerSet

src/cpp/ripple/ripple_LedgerAcquireMaster.h

@@ -1,9 +1,9 @@
 #ifndef RIPPLE_LEDGERACQUIREMASTER_H
 #define RIPPLE_LEDGERACQUIREMASTER_H
 
-/** Manages the acquisition of ledgers.
+/** Manages the lifetime of inbound ledgers.
 */
-// VFALCO TODO Rename to IncomingLedgerManager
+// VFALCO TODO Rename to InboundLedgerManager
 // VFALCO TODO Create abstract interface
 class LedgerAcquireMaster
 {

src/cpp/ripple/ripple_SHAMap.h

@@ -24,7 +24,7 @@ public:
 
 public:
 	// build new map
-	SHAMap(SHAMapType t, uint32 seq = 1);
+	explicit SHAMap (SHAMapType t, uint32 seq = 1);
 	SHAMap (SHAMapType t, uint256 const& hash);
 
 	~SHAMap() { mState = smsInvalid; }