Code refactor #291
No reviewers
Labels
No labels
area: devops
area: discovery
area: docs
area: livestream
area: proposal
consider soon
Epic
good first issue
hacktoberfest
hard fork
help wanted
icebox
Invalid
level: 0
level: 1
level: 2
level: 3
level: 4
needs: exploration
needs: grooming
needs: priority
needs: repro
needs: tech design
on hold
priority: blocker
priority: high
priority: low
priority: medium
resilience
soft fork
Tom's Wishlist
type: bug
type: discussion
type: improvement
type: new feature
type: refactor
type: task
type: testing
unplanned
work in progress
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: LBRYCommunity/lbrycrd#291
Loading…
Add table
Reference in a new issue
No description provided.
Delete branch "prefixtrie"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Signed-off-by: Anthony Fieroni bvbfan@abv.bg
This code refactoring aims to templatize all doublets algorithms addClaim/SupportToQueque, removeClaim/SupportFromQueue etc. to make code base well readable and better maintainable. We have issues with partial bug fixes due to same algorithm on more than one place, so this PR tends to remove that code duplication.
-> decltypeis c++14, no?@ -44,0 +56,4 @@template <typename K, typename V>bool equals(const std::pair<K, V>& pair, const CNameOutPointType& point){return pair.first == point.name && pair.second.outPoint == point.outPoint;Can we make CSupportValue and CClaimValue have the same base class that has the name and outpoint so that we don't have to use templated methods to work with them?
@ -245,4 +286,3 @@}return count;}how does this method name imply support?
Why is this stubbed method here?
@ -701,2 +741,4 @@}template <typename T>T CClaimTrieCacheBase::add(const std::string& name, const COutPoint& outPoint, const uint160& claimId, CAmount nAmount, int nHeight)A recursive break is not an exceptional condition. This should use return values instead. try/catch should be extremely rare and the catch should never be the expected scenario.
@ -44,0 +56,4 @@template <typename K, typename V>bool equals(const std::pair<K, V>& pair, const CNameOutPointType& point){return pair.first == point.name && pair.second.outPoint == point.outPoint;We can but dynamic dispatch is runtime / slower vs static one. If we go for polymorphic approach we'll face same penalty as well as runtime check to determine actual type of parameter.
No, https://en.cppreference.com/w/cpp/language/decltype
@ -245,4 +286,3 @@}return count;}The idea of meta programming is determining the type in compile time, i.e. using a function with unwanted type can fail compile time. The stubs are functions that will fail at compile time i.e. incorrect usage, supportedType will fail compilation for all type but claim and support.
@ -701,2 +741,4 @@}template <typename T>T CClaimTrieCacheBase::add(const std::string& name, const COutPoint& outPoint, const uint160& claimId, CAmount nAmount, int nHeight)It is
35a987bf2b (diff-f8129adb47c1b30eac7d2795b17883edR427)That's easy way to stop recursion, it does not affect other exceptions.
@ -245,4 +286,3 @@}return count;}Differentiating methods based on return value is not a good practice. Most computer languages don't even allow this; they don't include the return value type in the method footprint.
@ -701,2 +741,4 @@}template <typename T>T CClaimTrieCacheBase::add(const std::string& name, const COutPoint& outPoint, const uint160& claimId, CAmount nAmount, int nHeight)How is it easier than using a return value?
@ -44,0 +56,4 @@template <typename K, typename V>bool equals(const std::pair<K, V>& pair, const CNameOutPointType& point){return pair.first == point.name && pair.second.outPoint == point.outPoint;Taking a base class as a parameter doesn't automatically make a virtual function, no?
@ -245,4 +286,3 @@}return count;}It's perfectly valid in C++ and it's one of must have features to meta programming.
@ -701,2 +741,4 @@}template <typename T>T CClaimTrieCacheBase::add(const std::string& name, const COutPoint& outPoint, const uint160& claimId, CAmount nAmount, int nHeight)It breaks the stack without unwinding, i.e. it does not pop all stack pushes and the stack is unwind when catch appears. https://en.wikipedia.org/wiki/Call_stack#Unwinding
@ -44,0 +56,4 @@template <typename K, typename V>bool equals(const std::pair<K, V>& pair, const CNameOutPointType& point){return pair.first == point.name && pair.second.outPoint == point.outPoint;Yes, there is not automatically virtual functions, but you get the base class at some point of implementation you should what exactly is this type, claim or support without virtual dispatch you can't do it.
@ -44,0 +56,4 @@template <typename K, typename V>bool equals(const std::pair<K, V>& pair, const CNameOutPointType& point){return pair.first == point.name && pair.second.outPoint == point.outPoint;The C approach is same type for both (claim and support) with field in (enum in most cases) that describe the actual type.
"process" is pretty vague. Do we have a more specific name? And this is just to allow a few lines of code reuse between validation and hash computation?
@ -212,3 +186,4 @@for (auto claim : it->claims) {if (claim.nHeight + expirationTime() <= nNextHeight)continue;This has a bug in that it modifies the item we're about to insert. That other variable was there on purpose.
Same here: the other variable is necessary.
@ -212,3 +186,4 @@for (auto claim : it->claims) {if (claim.nHeight + expirationTime() <= nNextHeight)continue;Actually it isn't, because we know that we remove same support then re-add it under new name, no changes in value.
Suggest better naming, i'll update it.
@ -212,3 +186,4 @@for (auto claim : it->claims) {if (claim.nHeight + expirationTime() <= nNextHeight)continue;But better to iterate through copy rather than reference.
Missing an
==There is no
==here, it assigns tip height to nNextHeight then to base one.