refactor: simplify `FormatSubVersion` using strprintf/Join #30098

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202405-refactor-simplify-FormatSubVersion changing 1 files +4 −14

theStack commented at 9:30 PM on May 13, 2024: contributor

Rather than using std::ostringstream and manually joining the comments, use strprintf and our own Join helper.
DrahtBot commented at 9:30 PM on May 13, 2024: contributor



The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Code Coverage

For detailed information about the code coverage, see the test coverage report.



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK TheCharlatan, hebasto, maflcko, tdb3

Concept ACK kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
DrahtBot added the label Refactoring on May 13, 2024

Type	Reviewers
ACK	TheCharlatan, hebasto, maflcko, tdb3
Concept ACK	kevkevinpal

refactor: simplify `FormatSubVersion` using strprintf/Join

Rather than using std::ostringstream and manually joining the
comments, use strprintf and our own `Join` helper.

12d82817bf

theStack force-pushed on May 13, 2024
TheCharlatan approved
TheCharlatan commented at 9:16 AM on May 14, 2024: contributor

tACK 12d82817bf32396b58c8c65645012def606680b6
in src/clientversion.cpp:68 in 12d82817bf
```
  76 | -        ss << ")";
  77 | -    }
  78 | -    ss << "/";
  79 | -    return ss.str();
  80 | +    std::string comments_str;
  81 | +    if (!comments.empty()) comments_str = strprintf("(%s)", Join(comments, "; "));
```
hebasto commented at 9:42 AM on May 14, 2024:

nit: Placing a non-trivial if statement body on its own line can be useful for code coverage and debugging.
maflcko commented at 11:11 AM on May 14, 2024:

why is the if needed at all? Can't this whole function be a one-liner?

return strprintf("/%s:%s%s/", name, FormatVersion(nClientVersion), Join(comments, "; ");
theStack commented at 11:20 AM on May 14, 2024:

why is the if needed at all? Can't this whole function be a one-liner?

return strprintf("/%s:%s%s/", name, FormatVersion(nClientVersion), Join(comments, "; ");

Using this expression would miss the parentheses around the comments; they should only be there if there are comments at all. The alternative is using a single return expression and the ternary operator, but I found this slightly less readable.
hebasto approved
hebasto commented at 9:42 AM on May 14, 2024: member

ACK 12d82817bf32396b58c8c65645012def606680b6, I have reviewed the code and it looks OK.
maflcko commented at 11:26 AM on May 14, 2024: member

utACK 12d82817bf32396b58c8c65645012def606680b6
tdb3 approved
tdb3 commented at 1:13 AM on May 15, 2024: contributor

ACK for 12d82817bf32396b58c8c65645012def606680b6.

Thank you. Making code more concise and readable is much appreciated. Built and ran unit tests (including util_tests/test_FormatSubVersion) and functionals. All passed.

A one/same-line if aligns with the coding style (https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c).

If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces.
kevkevinpal commented at 1:56 AM on May 15, 2024: contributor

utACK #30084.
fanquake merged this on May 15, 2024
fanquake closed this on May 15, 2024
theStack deleted the branch on May 15, 2024
bitcoin locked this on May 15, 2025

Contributors

Labels

refactor: simplify `FormatSubVersion` using strprintf/Join #30098

Code Coverage

Reviews