From 234b754038c2f66e8f0394b5afe7fa6dada3bee7 Mon Sep 17 00:00:00 2001 From: Nik Bougalis Date: Fri, 3 Sep 2021 03:58:45 -0700 Subject: [PATCH] Eliminate code duplication and improve documentation: The legacy functions `cdirFirst` and `dirFirst` were mostly identical; the differences were only type-related. The same situation existed with `cdirNext` and `dirNext`. This commit removes the duplicated code by introducing new template functions that abstract away the differences that are present between each pair of functions. This commit also improves the naming of function arguments, helping to elucidate their purpose & use and to make the code self-documenting. --- src/ripple/app/misc/NetworkOPs.cpp | 16 +- src/ripple/app/tx/impl/BookTip.cpp | 2 +- src/ripple/app/tx/impl/DeleteAccount.cpp | 14 +- src/ripple/ledger/View.h | 96 ++++++---- src/ripple/ledger/impl/BookDirs.cpp | 12 +- src/ripple/ledger/impl/View.cpp | 226 ++++++++++++----------- src/test/rpc/Book_test.cpp | 7 +- 7 files changed, 187 insertions(+), 186 deletions(-) diff --git a/src/ripple/app/misc/NetworkOPs.cpp b/src/ripple/app/misc/NetworkOPs.cpp index cae1b6163f..b29133b1f6 100644 --- a/src/ripple/app/misc/NetworkOPs.cpp +++ b/src/ripple/app/misc/NetworkOPs.cpp @@ -3410,13 +3410,7 @@ NetworkOPsImp::getBookPage( uTipIndex = sleOfferDir->key(); saDirRate = amountFromQuality(getQuality(uTipIndex)); - cdirFirst( - view, - uTipIndex, - sleOfferDir, - uBookEntry, - offerIndex, - viewJ); + cdirFirst(view, uTipIndex, sleOfferDir, uBookEntry, offerIndex); JLOG(m_journal.trace()) << "getBookPage: uTipIndex=" << uTipIndex; @@ -3536,13 +3530,7 @@ NetworkOPsImp::getBookPage( JLOG(m_journal.warn()) << "Missing offer"; } - if (!cdirNext( - view, - uTipIndex, - sleOfferDir, - uBookEntry, - offerIndex, - viewJ)) + if (!cdirNext(view, uTipIndex, sleOfferDir, uBookEntry, offerIndex)) { bDirectAdvance = true; } diff --git a/src/ripple/app/tx/impl/BookTip.cpp b/src/ripple/app/tx/impl/BookTip.cpp index 2ec2c7b44e..88bfd1d516 100644 --- a/src/ripple/app/tx/impl/BookTip.cpp +++ b/src/ripple/app/tx/impl/BookTip.cpp @@ -55,7 +55,7 @@ BookTip::step(beast::Journal j) unsigned int di = 0; std::shared_ptr dir; - if (dirFirst(view_, *first_page, dir, di, m_index, j)) + if (dirFirst(view_, *first_page, dir, di, m_index)) { m_dir = dir->key(); m_entry = view_.peek(keylet::offer(m_index)); diff --git a/src/ripple/app/tx/impl/DeleteAccount.cpp b/src/ripple/app/tx/impl/DeleteAccount.cpp index 26fb2236c1..d0605e08e6 100644 --- a/src/ripple/app/tx/impl/DeleteAccount.cpp +++ b/src/ripple/app/tx/impl/DeleteAccount.cpp @@ -198,12 +198,7 @@ DeleteAccount::preclaim(PreclaimContext const& ctx) uint256 dirEntry{beast::zero}; if (!cdirFirst( - ctx.view, - ownerDirKeylet.key, - sleDirNode, - uDirEntry, - dirEntry, - ctx.j)) + ctx.view, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)) // Account has no directory at all. This _should_ have been caught // by the dirIsEmpty() check earlier, but it's okay to catch it here. return tesSUCCESS; @@ -237,7 +232,7 @@ DeleteAccount::preclaim(PreclaimContext const& ctx) return tefTOO_BIG; } while (cdirNext( - ctx.view, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry, ctx.j)); + ctx.view, ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)); return tesSUCCESS; } @@ -261,8 +256,7 @@ DeleteAccount::doApply() uint256 dirEntry{beast::zero}; if (view().exists(ownerDirKeylet) && - dirFirst( - view(), ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry, j_)) + dirFirst(view(), ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)) { do { @@ -324,7 +318,7 @@ DeleteAccount::doApply() uDirEntry = 0; } while (dirNext( - view(), ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry, j_)); + view(), ownerDirKeylet.key, sleDirNode, uDirEntry, dirEntry)); } // Transfer any XRP remaining after the fee is paid to the destination: diff --git a/src/ripple/ledger/View.h b/src/ripple/ledger/View.h index d74a0aec67..737fdee382 100644 --- a/src/ripple/ledger/View.h +++ b/src/ripple/ledger/View.h @@ -125,30 +125,6 @@ transferRate(ReadView const& view, AccountID const& issuer); [[nodiscard]] bool dirIsEmpty(ReadView const& view, Keylet const& k); -// Return the first entry and advance uDirEntry. -// <-- true, if had a next entry. -// VFALCO Fix these clumsy routines with an iterator -bool -cdirFirst( - ReadView const& view, - uint256 const& uRootIndex, // --> Root of directory. - std::shared_ptr& sleNode, // <-> current node - unsigned int& uDirEntry, // <-- next entry - uint256& uEntryIndex, // <-- The entry, if available. Otherwise, zero. - beast::Journal j); - -// Return the current entry and advance uDirEntry. -// <-- true, if had a next entry. -// VFALCO Fix these clumsy routines with an iterator -bool -cdirNext( - ReadView const& view, - uint256 const& uRootIndex, // --> Root of directory - std::shared_ptr& sleNode, // <-> current node - unsigned int& uDirEntry, // <-> next entry - uint256& uEntryIndex, // <-- The entry, if available. Otherwise, zero. - beast::Journal j); - // Return the list of enabled amendments [[nodiscard]] std::set getEnabledAmendments(ReadView const& view); @@ -222,29 +198,69 @@ adjustOwnerCount( std::int32_t amount, beast::Journal j); -// Return the first entry and advance uDirEntry. -// <-- true, if had a next entry. -// VFALCO Fix these clumsy routines with an iterator +/** @{ */ +/** Returns the first entry in the directory, advancing the index + + @deprecated These are legacy function that are considered deprecated + and will soon be replaced with an iterator-based model + that is easier to use. You should not use them in new code. + + @param view The view against which to operate + @param root The root (i.e. first page) of the directory to iterate + @param page The current page + @param index The index inside the current page + @param entry The entry at the current index + + @return true if the directory isn't empty; false otherwise + */ +bool +cdirFirst( + ReadView const& view, + uint256 const& root, + std::shared_ptr& page, + unsigned int& index, + uint256& entry); + bool dirFirst( ApplyView& view, - uint256 const& uRootIndex, // --> Root of directory. - std::shared_ptr& sleNode, // <-> current node - unsigned int& uDirEntry, // <-- next entry - uint256& uEntryIndex, // <-- The entry, if available. Otherwise, zero. - beast::Journal j); + uint256 const& root, + std::shared_ptr& page, + unsigned int& index, + uint256& entry); +/** @} */ + +/** @{ */ +/** Returns the next entry in the directory, advancing the index + + @deprecated These are legacy function that are considered deprecated + and will soon be replaced with an iterator-based model + that is easier to use. You should not use them in new code. + + @param view The view against which to operate + @param root The root (i.e. first page) of the directory to iterate + @param page The current page + @param index The index inside the current page + @param entry The entry at the current index + + @return true if the directory isn't empty; false otherwise + */ +bool +cdirNext( + ReadView const& view, + uint256 const& root, + std::shared_ptr& page, + unsigned int& index, + uint256& entry); -// Return the current entry and advance uDirEntry. -// <-- true, if had a next entry. -// VFALCO Fix these clumsy routines with an iterator bool dirNext( ApplyView& view, - uint256 const& uRootIndex, // --> Root of directory - std::shared_ptr& sleNode, // <-> current node - unsigned int& uDirEntry, // <-> next entry - uint256& uEntryIndex, // <-- The entry, if available. Otherwise, zero. - beast::Journal j); + uint256 const& root, + std::shared_ptr& page, + unsigned int& index, + uint256& entry); +/** @} */ [[nodiscard]] std::function describeOwnerDir(AccountID const& account); diff --git a/src/ripple/ledger/impl/BookDirs.cpp b/src/ripple/ledger/impl/BookDirs.cpp index 5f31718758..f8589d5575 100644 --- a/src/ripple/ledger/impl/BookDirs.cpp +++ b/src/ripple/ledger/impl/BookDirs.cpp @@ -33,13 +33,7 @@ BookDirs::BookDirs(ReadView const& view, Book const& book) assert(root_ != beast::zero); if (key_ != beast::zero) { - if (!cdirFirst( - *view_, - key_, - sle_, - entry_, - index_, - beast::Journal{beast::Journal::getNullSink()})) + if (!cdirFirst(*view_, key_, sle_, entry_, index_)) { assert(false); } @@ -96,7 +90,7 @@ BookDirs::const_iterator::operator++() using beast::zero; assert(index_ != zero); - if (!cdirNext(*view_, cur_key_, sle_, entry_, index_, j_)) + if (!cdirNext(*view_, cur_key_, sle_, entry_, index_)) { if (index_ != 0 || (cur_key_ = @@ -106,7 +100,7 @@ BookDirs::const_iterator::operator++() entry_ = 0; index_ = zero; } - else if (!cdirFirst(*view_, cur_key_, sle_, entry_, index_, j_)) + else if (!cdirFirst(*view_, cur_key_, sle_, entry_, index_)) { assert(false); } diff --git a/src/ripple/ledger/impl/View.cpp b/src/ripple/ledger/impl/View.cpp index ec4bfab332..e7b0334323 100644 --- a/src/ripple/ledger/impl/View.cpp +++ b/src/ripple/ledger/impl/View.cpp @@ -34,6 +34,126 @@ namespace ripple { +namespace detail { + +template < + class V, + class N, + class = std::enable_if_t< + std::is_same_v, SLE> && + std::is_base_of_v>> +bool +internalDirNext( + V& view, + uint256 const& root, + std::shared_ptr& page, + unsigned int& index, + uint256& entry) +{ + auto const& svIndexes = page->getFieldV256(sfIndexes); + assert(index <= svIndexes.size()); + + if (index >= svIndexes.size()) + { + auto const next = page->getFieldU64(sfIndexNext); + + if (!next) + { + entry.zero(); + return false; + } + + if constexpr (std::is_const_v) + page = view.read(keylet::page(root, next)); + else + page = view.peek(keylet::page(root, next)); + + assert(page); + + if (!page) + return false; + + index = 0; + + return internalDirNext(view, root, page, index, entry); + } + + entry = svIndexes[index++]; + return true; +} + +template < + class V, + class N, + class = std::enable_if_t< + std::is_same_v, SLE> && + std::is_base_of_v>> +bool +internalDirFirst( + V& view, + uint256 const& root, + std::shared_ptr& page, + unsigned int& index, + uint256& entry) +{ + if constexpr (std::is_const_v) + page = view.read(keylet::page(root)); + else + page = view.peek(keylet::page(root)); + + assert(page); + + index = 0; + + return internalDirNext(view, root, page, index, entry); +} + +} // namespace detail + +bool +dirFirst( + ApplyView& view, + uint256 const& root, + std::shared_ptr& page, + unsigned int& index, + uint256& entry) +{ + return detail::internalDirFirst(view, root, page, index, entry); +} + +bool +dirNext( + ApplyView& view, + uint256 const& root, + std::shared_ptr& page, + unsigned int& index, + uint256& entry) +{ + return detail::internalDirNext(view, root, page, index, entry); +} + +bool +cdirFirst( + ReadView const& view, + uint256 const& root, + std::shared_ptr& page, + unsigned int& index, + uint256& entry) +{ + return detail::internalDirFirst(view, root, page, index, entry); +} + +bool +cdirNext( + ReadView const& view, + uint256 const& root, + std::shared_ptr& page, + unsigned int& index, + uint256& entry) +{ + return detail::internalDirNext(view, root, page, index, entry); +} + //------------------------------------------------------------------------------ // // Observers @@ -480,59 +600,6 @@ dirIsEmpty(ReadView const& view, Keylet const& k) return sleNode->getFieldU64(sfIndexNext) == 0; } -bool -cdirFirst( - ReadView const& view, - uint256 const& uRootIndex, // --> Root of directory. - std::shared_ptr& sleNode, // <-> current node - unsigned int& uDirEntry, // <-- next entry - uint256& uEntryIndex, // <-- The entry, if available. Otherwise, zero. - beast::Journal j) -{ - sleNode = view.read(keylet::page(uRootIndex)); - uDirEntry = 0; - assert(sleNode); // Never probe for directories. - return cdirNext(view, uRootIndex, sleNode, uDirEntry, uEntryIndex, j); -} - -bool -cdirNext( - ReadView const& view, - uint256 const& uRootIndex, // --> Root of directory - std::shared_ptr& sleNode, // <-> current node - unsigned int& uDirEntry, // <-> next entry - uint256& uEntryIndex, // <-- The entry, if available. Otherwise, zero. - beast::Journal j) -{ - auto const& svIndexes = sleNode->getFieldV256(sfIndexes); - assert(uDirEntry <= svIndexes.size()); - if (uDirEntry >= svIndexes.size()) - { - auto const uNodeNext = sleNode->getFieldU64(sfIndexNext); - if (!uNodeNext) - { - uEntryIndex.zero(); - return false; - } - auto const sleNext = view.read(keylet::page(uRootIndex, uNodeNext)); - uDirEntry = 0; - if (!sleNext) - { - // This should never happen - JLOG(j.fatal()) << "Corrupt directory: index:" << uRootIndex - << " next:" << uNodeNext; - return false; - } - sleNode = sleNext; - return cdirNext(view, uRootIndex, sleNode, uDirEntry, uEntryIndex, j); - } - uEntryIndex = svIndexes[uDirEntry++]; - JLOG(j.trace()) << "dirNext:" - << " uDirEntry=" << uDirEntry - << " uEntryIndex=" << uEntryIndex; - return true; -} - std::set getEnabledAmendments(ReadView const& view) { @@ -660,59 +727,6 @@ adjustOwnerCount( view.update(sle); } -bool -dirFirst( - ApplyView& view, - uint256 const& uRootIndex, // --> Root of directory. - std::shared_ptr& sleNode, // <-> current node - unsigned int& uDirEntry, // <-- next entry - uint256& uEntryIndex, // <-- The entry, if available. Otherwise, zero. - beast::Journal j) -{ - sleNode = view.peek(keylet::page(uRootIndex)); - uDirEntry = 0; - assert(sleNode); // Never probe for directories. - return dirNext(view, uRootIndex, sleNode, uDirEntry, uEntryIndex, j); -} - -bool -dirNext( - ApplyView& view, - uint256 const& uRootIndex, // --> Root of directory - std::shared_ptr& sleNode, // <-> current node - unsigned int& uDirEntry, // <-> next entry - uint256& uEntryIndex, // <-- The entry, if available. Otherwise, zero. - beast::Journal j) -{ - auto const& svIndexes = sleNode->getFieldV256(sfIndexes); - assert(uDirEntry <= svIndexes.size()); - if (uDirEntry >= svIndexes.size()) - { - auto const uNodeNext = sleNode->getFieldU64(sfIndexNext); - if (!uNodeNext) - { - uEntryIndex.zero(); - return false; - } - auto const sleNext = view.peek(keylet::page(uRootIndex, uNodeNext)); - uDirEntry = 0; - if (!sleNext) - { - // This should never happen - JLOG(j.fatal()) << "Corrupt directory: index:" << uRootIndex - << " next:" << uNodeNext; - return false; - } - sleNode = sleNext; - return dirNext(view, uRootIndex, sleNode, uDirEntry, uEntryIndex, j); - } - uEntryIndex = svIndexes[uDirEntry++]; - JLOG(j.trace()) << "dirNext:" - << " uDirEntry=" << uDirEntry - << " uEntryIndex=" << uEntryIndex; - return true; -} - std::function describeOwnerDir(AccountID const& account) { diff --git a/src/test/rpc/Book_test.cpp b/src/test/rpc/Book_test.cpp index 93eb2849a7..fcc0017f53 100644 --- a/src/test/rpc/Book_test.cpp +++ b/src/test/rpc/Book_test.cpp @@ -41,12 +41,7 @@ class Book_test : public beast::unit_test::suite uint256 offerIndex; unsigned int bookEntry; cdirFirst( - *view, - sleOfferDir->key(), - sleOfferDir, - bookEntry, - offerIndex, - env.journal); + *view, sleOfferDir->key(), sleOfferDir, bookEntry, offerIndex); auto sleOffer = view->read(keylet::offer(offerIndex)); dir = to_string(sleOffer->getFieldH256(sfBookDirectory)); }