Code refactor #291

Merged
bvbfan merged 4 commits from prefixtrie into master 2019-07-29 17:23:57 +02:00
bvbfan commented 2019-07-02 18:00:19 +02:00 (Migrated from github.com)

Signed-off-by: Anthony Fieroni bvbfan@abv.bg

Signed-off-by: Anthony Fieroni <bvbfan@abv.bg>
bvbfan commented 2019-07-02 18:05:02 +02:00 (Migrated from github.com)

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.

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.
BrannonKing (Migrated from github.com) requested changes 2019-07-09 17:40:24 +02:00
BrannonKing (Migrated from github.com) commented 2019-07-09 17:31:04 +02:00

-> decltype is c++14, no?

`-> decltype` is 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;
BrannonKing (Migrated from github.com) commented 2019-07-09 17:30:15 +02:00

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?

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;
}
BrannonKing (Migrated from github.com) commented 2019-07-09 17:32:27 +02:00

how does this method name imply support?

how does this method name imply support?
BrannonKing (Migrated from github.com) commented 2019-07-09 17:33:25 +02:00

Why is this stubbed method here?

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)
BrannonKing (Migrated from github.com) commented 2019-07-09 17:37:00 +02:00

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.

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.
bvbfan (Migrated from github.com) reviewed 2019-07-09 19:32:18 +02:00
@ -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;
bvbfan (Migrated from github.com) commented 2019-07-09 19:32:17 +02:00

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.

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.
bvbfan (Migrated from github.com) reviewed 2019-07-09 19:32:39 +02:00
bvbfan (Migrated from github.com) commented 2019-07-09 19:32:39 +02:00
No, https://en.cppreference.com/w/cpp/language/decltype
bvbfan (Migrated from github.com) reviewed 2019-07-09 19:33:56 +02:00
@ -245,4 +286,3 @@
}
return count;
}
bvbfan (Migrated from github.com) commented 2019-07-09 19:33:55 +02:00
std::pair<const int, std::vector<queueEntryType<CSupportValue>>>*
                                                -------------
``` std::pair<const int, std::vector<queueEntryType<CSupportValue>>>* ------------- ````
bvbfan (Migrated from github.com) reviewed 2019-07-09 19:37:27 +02:00
bvbfan (Migrated from github.com) commented 2019-07-09 19:37:26 +02:00

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.

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<T> will fail compilation for all type but claim and support.
bvbfan (Migrated from github.com) reviewed 2019-07-09 19:39:33 +02:00
@ -701,2 +741,4 @@
}
template <typename T>
T CClaimTrieCacheBase::add(const std::string& name, const COutPoint& outPoint, const uint160& claimId, CAmount nAmount, int nHeight)
bvbfan (Migrated from github.com) commented 2019-07-09 19:39:33 +02:00

It is 35a987bf2b (diff-f8129adb47c1b30eac7d2795b17883edR427)
That's easy way to stop recursion, it does not affect other exceptions.

It is https://github.com/lbryio/lbrycrd/pull/291/commits/35a987bf2b7bf7b4d4f5b08297c5a45a4d467492#diff-f8129adb47c1b30eac7d2795b17883edR427 That's easy way to stop recursion, it does not affect other exceptions.
BrannonKing (Migrated from github.com) reviewed 2019-07-11 20:55:57 +02:00
@ -245,4 +286,3 @@
}
return count;
}
BrannonKing (Migrated from github.com) commented 2019-07-11 20:55:57 +02:00

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.

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.
BrannonKing (Migrated from github.com) reviewed 2019-07-11 20:57:13 +02:00
@ -701,2 +741,4 @@
}
template <typename T>
T CClaimTrieCacheBase::add(const std::string& name, const COutPoint& outPoint, const uint160& claimId, CAmount nAmount, int nHeight)
BrannonKing (Migrated from github.com) commented 2019-07-11 20:57:12 +02:00

How is it easier than using a return value?

How is it easier than using a return value?
BrannonKing (Migrated from github.com) reviewed 2019-07-11 20:57:54 +02:00
@ -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;
BrannonKing (Migrated from github.com) commented 2019-07-11 20:57:54 +02:00

Taking a base class as a parameter doesn't automatically make a virtual function, no?

Taking a base class as a parameter doesn't automatically make a virtual function, no?
bvbfan (Migrated from github.com) reviewed 2019-07-12 08:51:07 +02:00
@ -245,4 +286,3 @@
}
return count;
}
bvbfan (Migrated from github.com) commented 2019-07-12 08:51:07 +02:00

It's perfectly valid in C++ and it's one of must have features to meta programming.

It's perfectly valid in C++ and it's one of must have features to meta programming.
bvbfan (Migrated from github.com) reviewed 2019-07-12 08:56:44 +02:00
@ -701,2 +741,4 @@
}
template <typename T>
T CClaimTrieCacheBase::add(const std::string& name, const COutPoint& outPoint, const uint160& claimId, CAmount nAmount, int nHeight)
bvbfan (Migrated from github.com) commented 2019-07-12 08:56:44 +02:00

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

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
bvbfan (Migrated from github.com) reviewed 2019-07-12 08:58:54 +02:00
@ -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;
bvbfan (Migrated from github.com) commented 2019-07-12 08:58:53 +02:00

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.

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.
bvbfan (Migrated from github.com) reviewed 2019-07-15 07:52:01 +02:00
@ -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;
bvbfan (Migrated from github.com) commented 2019-07-15 07:52:01 +02:00

The C approach is same type for both (claim and support) with field in (enum in most cases) that describe the actual type.

The C approach is same type for both (claim and support) with field in (enum in most cases) that describe the actual type.
BrannonKing (Migrated from github.com) requested changes 2019-07-16 18:48:53 +02:00
BrannonKing (Migrated from github.com) commented 2019-07-16 18:45:14 +02:00

"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?

"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;
BrannonKing (Migrated from github.com) commented 2019-07-16 18:42:09 +02:00

This has a bug in that it modifies the item we're about to insert. That other variable was there on purpose.

This has a bug in that it modifies the item we're about to insert. That other variable was there on purpose.
BrannonKing (Migrated from github.com) commented 2019-07-16 18:42:47 +02:00

Same here: the other variable is necessary.

Same here: the other variable is necessary.
bvbfan (Migrated from github.com) reviewed 2019-07-16 19:15:49 +02:00
@ -212,3 +186,4 @@
for (auto claim : it->claims) {
if (claim.nHeight + expirationTime() <= nNextHeight)
continue;
bvbfan (Migrated from github.com) commented 2019-07-16 19:15:49 +02:00

Actually it isn't, because we know that we remove same support then re-add it under new name, no changes in value.

Actually it isn't, because we know that we remove same support then re-add it under new name, no changes in value.
bvbfan (Migrated from github.com) reviewed 2019-07-16 19:16:24 +02:00
bvbfan (Migrated from github.com) commented 2019-07-16 19:16:23 +02:00

Suggest better naming, i'll update it.

Suggest better naming, i'll update it.
bvbfan (Migrated from github.com) reviewed 2019-07-16 20:20:22 +02:00
@ -212,3 +186,4 @@
for (auto claim : it->claims) {
if (claim.nHeight + expirationTime() <= nNextHeight)
continue;
bvbfan (Migrated from github.com) commented 2019-07-16 20:20:22 +02:00

But better to iterate through copy rather than reference.

But better to iterate through copy rather than reference.
BrannonKing (Migrated from github.com) reviewed 2019-07-19 21:57:35 +02:00
BrannonKing (Migrated from github.com) commented 2019-07-19 21:57:34 +02:00

Missing an ==

Missing an `==`
bvbfan (Migrated from github.com) reviewed 2019-07-20 16:42:18 +02:00
bvbfan (Migrated from github.com) commented 2019-07-20 16:42:18 +02:00

There is no == here, it assigns tip height to nNextHeight then to base one.

There is no `==` here, it assigns tip height to nNextHeight then to base one.
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: LBRYCommunity/lbrycrd#291
No description provided.