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.
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;
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 ?
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)) {
0 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
0double a = 1.2;
1uint64_t b;
2b = 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)) {
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()`.
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:
0for (int i = 0; i < 64; i++) {
1 uint64_t check = 1ull << i;
2 if (mask & check)
3 {
4 ...
5 }
6}
to:
0for (const auto& flag : serviceFlagsToStr(mask)) {
1 ...
2}
