Don't take two redundant arguments in serviceFlagToStr().
Introduce serviceFlagsToStr() which takes a mask (with more than one
bit set) and returns a vector of strings.
As a side effect this fixes an issue introduced in
#18165 due to which the GUI could
print something like UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]
instead of NETWORK & WITNESS.
Concept ACK.
216 | @@ -211,10 +217,23 @@ std::string serviceFlagToStr(const uint64_t mask, const int bit) 217 | stream.imbue(std::locale::classic()); 218 | stream << "UNKNOWN["; 219 | if (bit < 8) { 220 | - stream << mask; 221 | + stream << service_flag;
Why is this branch needed? 2^7 or similar should read just fine and help with consistency, no?
It was like this in the original code and I am trying to limit the amount of changes. Especially this would be user-visible change. Otherwise I agree that for consistency we may just print 2^N.
I do not know why it was done like that. Maybe with the assumption that e.g. 64 is more human-readable than 2^6, but 524288 is not more human-readable than 2^19.
It was introduced in https://github.com/bitcoin/bitcoin/commit/df77de8c2#diff-a459a939be4fe065dd5f64ab60bb30d0L842 @luke-jr ?
No, it predates that commit too.
759 | - uint64_t check = 1ull << i; 760 | - if (mask & check) 761 | - { 762 | - strList.append(QString::fromStdString(serviceFlagToStr(mask, i))); 763 | - } 764 | + for (const auto& flag : serviceFlagsToStr((uint64_t)mask)) {
for (const auto& flag : serviceFlagsToStr(uint64_t{mask})) {
nit: this is slightly preferable (assuming it compiles)
Thanks for the hint. I wasn't aware that
double a = 1.2;
uint64_t b;
b = uint64_t{a};
wouldn't compile due to type narrowing (whereas b = (uint64_t)a would compile).
In our case however the typecast is not needed, so I have removed it altogether.
847 | - for (int i = 0; i < 64; ++i) { 848 | - const uint64_t mask = 1ull << i; 849 | - if (services_n & mask) { 850 | - servicesNames.push_back(serviceFlagToStr(mask, i)); 851 | - } 852 | + for (const auto& flag : serviceFlagsToStr((uint64_t)services)) {
Same
ACK, just some questions.
You have 3 changes here, but only 1 commit. Each logical change should be a separate commit. So bugfix, refactor/simplification, and then new serviceFlagsToStr.
I don't think serviceFlagsToStr has any purpose though... both callers need to loop over the vector to transform it.
Don't take two redundant arguments in `serviceFlagToStr()`.
As a side effect this fixes an issue introduced in
https://github.com/bitcoin/bitcoin/pull/18165 due to which the GUI could
print something like `UNKNOWN[1033] & UNKNOWN[1033] & UNKNOWN[2^10]`
instead of `NETWORK & WITNESS`.
Introduce `serviceFlagsToStr()` which hides the internals of the bitmask
and simplifies callers of `serviceFlagToStr()`.
ACK 189ae0c38b7d4927c5c73b94664e9542b2b06ed9
You have 3 changes here, but only 1 commit. Each logical change should be a separate commit. So bugfix, refactor/simplification, and then new serviceFlagsToStr.
The refactor/simplification of changing serviceFlagToStr() to take 1 argument instead of 2 is also the bug fix. So we have 2 changes. I split it to two commits even though the second one overwrites parts of the first one.
I don't think serviceFlagsToStr has any purpose though... both callers need to loop over the vector to transform it.
Its purpose is to simplify the two callers which had duplicated code like:
for (int i = 0; i < 64; i++) {
uint64_t check = 1ull << i;
if (mask & check)
{
...
}
}
to:
for (const auto& flag : serviceFlagsToStr(mask)) {
...
}
Tested ACK 189ae0c38b7d4927c5c73b94664e9542b2b06ed9