While reviewing the safeguards against deliberate node isolation on the network by malicious actors, I found a good de-duplication candidate.
I think this form is much more legible (the type of cutoffs notwithstanding). ReverseCompareNodeTimeConnected is not included in the list since the cutoff size is a function of the remaining number of nodes in the candidate eviction set.
Concept ACK. Nice cleanup!
Concept ACK.
I would prefer to abstract the loop body into a function, and call it 4 times with the given arguments.
It should be easier to test and IMO understand.
utACK b52635e0b9db58095c21de1af5f5a0c74a85b63c. Change is an an improvement over existing code, but this can be simplified more. There is no need for std::function indirection, and no need for looping. All you need here is a plain generic function:
//! Remove last n elements from container after sorting, and return true if it is non-empty.
template<typename Container, typename Compare>
bool EraseLastN(Container& container, int n, Compare compare);
and
if (!(EraseLastN(vEvictionCandidates, 4, CompareNetGroupKeyed) ||
!(EraseLastN(vEvictionCandidates, 8, ReverseCompareNodeMinPingTime) ||
!(EraseLastN(vEvictionCandidates, 4, CompareNodeTXTime) ||
!(EraseLastN(vEvictionCandidates, 4, CompareNodeBlockTime)) {
return false
};
You might also want to use std::partial_sort instead of std::sort in the new code since a full sort isn't necessary.
Concept ACK; I prefer @ryanofsky's suggestion as well.
Concept ACK, nice cleanup. I too prefer @ryanofsky's version plus your comments :)
@ryanofsky - I was unaware of std::partial_sort, and while it seems perfectly suited to this use case, all the benchmarks I found comparing it to std::sort seemed to indicate it was slower. I'll leave it using std::sort for now.
Also since removeLastKElements is a no-op on an empty vector, I left it as a void return type and skipped the chained conditional return checks. I think this makes the code more readable.
Updated diff to include a little DRY/readability improvement at the end of the same function.
961 | @@ -962,6 +962,16 @@ static bool CompareNodeTXTime(const NodeEvictionCandidate &a, const NodeEviction 962 | return a.nTimeConnected > b.nTimeConnected; 963 | } 964 | 965 | + 966 | +//! Sort an array by the specified comparator, then remove the last K elements. 967 | +template<typename T> 968 | +static void removeLastKElements(std::vector<T> &elements, bool(*comparator)(const T&, const T&), size_t k)
Maybe don't require comparator to be function pointer. It'd be nice to allow comparator to be a lambda or function class:
template<typename T, typename Compare>
static void removeLastKElements(std::vector<T> &elements, Compare comp, size_t k)
{
std::sort(elements.begin(), elements.end(), comp);
...
}
That's a good point, should have pulled that from your example. Old habits die hard.
966 | +//! Sort an array by the specified comparator, then remove the last K elements. 967 | +template<typename T> 968 | +static void removeLastKElements(std::vector<T> &elements, bool(*comparator)(const T&, const T&), size_t k) 969 | +{ 970 | + std::sort(elements.begin(), elements.end(), comparator); 971 | + size_t cutSize = std::min(k, elements.size());
Naming convention in new code would call this cut_size. I would probably just inline this variable, though. Also I think it's a little weird how the code is using "remove" "cut" and "erase" interchangeably. I don't see why you wouldn't want to be consistent and just say "erase" everywhere.
I like k to refer to the K in the function name. I agree that the naming should be more consistent, switching to 'erase' across the board.
utACK 0bd38149bde46779b5d0823344c1187372062d13. Still looks good. Feel free to ignore new nitpicky comments.
967 | +template<typename T, typename Comparator> 968 | +static void eraseLastKElements(std::vector<T> &elements, Comparator comparator, size_t k) 969 | +{ 970 | + std::sort(elements.begin(), elements.end(), comparator); 971 | + size_t eraseSize = std::min(k, elements.size()); 972 | + elements.erase(elements.end() - eraseSize, elements.end());
Technically this introduces undefined behavior - we no longer return early if we're gonna call erase(end()), which is undefined.
Technically this introduces undefined behavior - we no longer return early if we're gonna call erase(end()), which is undefined.
This seems to be fine according to http://en.cppreference.com/w/cpp/container/vector/erase: "The iterator first does not need to be dereferenceable if first==last: erasing an empty range is a no-op."
Oops, indeed, somehow I missed that line.
utACK efa57568236cd7b3a80419e09e9babd546cbbda3
utACK efa57568236cd7b3a80419e09e9babd546cbbda3, but can you follow the function naming convention (EraseLastKElements)?
re-utACK 5ce7cb95180b7d8873dae91d8370bc7ea69cd2a7, only change is the function name.
utACK 5ce7cb9
post merge utACK 5ce7cb95180b7d8873dae91d8370bc7ea69cd2a7