Refactor: Replace fprintf with tfm::format

MarcoFalke commented at 1:34 pm on June 13, 2019: member

This should be a refactor except in the cases where we use the wrong format specifier [1], in which case this patch is a bug fix.

[1] : e.g. depends: Add libevent compatibility patch for windows #8730

tinyformat: Add doc to Bitcoin Core specific strprintf fa72a64b90

MarcoFalke added the label Refactoring on Jun 13, 2019

MarcoFalke added this to the milestone 0.19.0 on Jun 13, 2019

MarcoFalke force-pushed on Jun 13, 2019

laanwj commented at 1:47 pm on June 13, 2019: member

Nice. Concept ACK. Wasn’t aware we had so many uses of fprintf. We should probably change the linter to prevent it from being used again.

laanwj commented at 1:51 pm on June 13, 2019: member

This might do it:

 0diff --git a/test/lint/lint-locale-dependence.sh b/test/lint/lint-locale-dependence.sh
 1index 2b6c78c2c841b7cab90ce11b831ea1051118029e..b0c5ce39b1aba1c52b9b23c7451ac46d33cc6c67 100755
 2--- a/test/lint/lint-locale-dependence.sh
 3+++ b/test/lint/lint-locale-dependence.sh
 4@@ -8,7 +8,6 @@ KNOWN_VIOLATIONS=(
 5     "src/dbwrapper.cpp:.*vsnprintf"
 6     "src/httprpc.cpp.*trim"
 7     "src/init.cpp:.*atoi"
 8-    "src/init.cpp:.*fprintf"
 9     "src/qt/rpcconsole.cpp:.*atoi"
10     "src/rest.cpp:.*strtol"
11     "src/test/dbwrapper_tests.cpp:.*snprintf"
12@@ -189,8 +188,7 @@ GIT_GREP_OUTPUT=$(git grep -E "[^a-zA-Z0-9_\`'\"<>](${REGEXP_LOCALE_DEPENDENT_FU
13 EXIT_CODE=0
14 for LOCALE_DEPENDENT_FUNCTION in "${LOCALE_DEPENDENT_FUNCTIONS[@]}"; do
15     MATCHES=$(grep -E "[^a-zA-Z0-9_\`'\"<>]${LOCALE_DEPENDENT_FUNCTION}(_r|_s)?[^a-zA-Z0-9_\`'\"<>]" <<< "${GIT_GREP_OUTPUT}" | \
16-        grep -vE "\.(c|cpp|h):\s*(//|\*|/\*|\").*${LOCALE_DEPENDENT_FUNCTION}" | \
17-        grep -vE 'fprintf\(.*(stdout|stderr)')
18+        grep -vE "\.(c|cpp|h):\s*(//|\*|/\*|\").*${LOCALE_DEPENDENT_FUNCTION}")
19     if [[ ${REGEXP_IGNORE_EXTERNAL_DEPENDENCIES} != "" ]]; then
20         MATCHES=$(grep -vE "${REGEXP_IGNORE_EXTERNAL_DEPENDENCIES}" <<< "${MATCHES}")
21     fi

MarcoFalke force-pushed on Jun 13, 2019

scripted-diff: Replace fprintf with tfm::format

-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended -e 's/fprintf\(std(err|out), /tfm::format(std::c\1, /g' $(git grep -l 'fprintf(' -- ':(exclude)src/crypto' ':(exclude)src/leveldb' ':(exclude)src/univalue' ':(exclude)src/secp256k1')
-END VERIFY SCRIPT-

fixup! scripted-diff: Replace fprintf with tfm::format

fac03ec43a

MarcoFalke force-pushed on Jun 13, 2019

practicalswift commented at 2:43 pm on June 13, 2019: contributor

Concept ACK

DrahtBot commented at 2:49 pm on June 13, 2019: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15936 (WIP: Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)
#15894 (Remove duplicated “Error: " prefix in logs by hebasto)
#15864 (Fix datadir handling by hebasto)
#14866 (Improve property evaluation way in bitcoin.conf by AkioNak)
#10102 ([experimental] Multiprocess bitcoin by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jamesob commented at 3:32 pm on June 13, 2019: member

Concept ACK, will test in a bit.

Replace remaining fprintf with tfm::format manually fa8f195195

MarcoFalke force-pushed on Jun 13, 2019

promag commented at 9:43 pm on June 13, 2019: member

ACK fa8f195195945ce6258199af0461e3fbfbc1236d. Ideally this should be rebased before merge.

practicalswift commented at 9:51 pm on June 13, 2019: contributor

utACK fa8f195195945ce6258199af0461e3fbfbc1236d

practicalswift commented at 8:03 am on June 14, 2019: contributor

When reviewing please note that tfm::format in contrast to fprintf may throw (tinyformat::format_error). That is not necessarily a problem obviously, but it is worth to have in mind when analysing the change in possible code paths from this change.

in src/bitcoin-tx.cpp:110 in fa8f195195

103@@ -104,10 +104,10 @@ static int AppInitRawTx(int argc, char* argv[])
104             "\n";
105         strUsage += gArgs.GetHelpMessage();
106 
107-        fprintf(stdout, "%s", strUsage.c_str());
108+        tfm::format(std::cout, "%s", strUsage.c_str());
109 
110         if (argc < 2) {
111-            fprintf(stderr, "Error: too few parameters\n");
112+            tfm::format(std::cerr, "Error: too few parameters\n");

Empact commented at 1:01 pm on June 14, 2019:

nit: a few of these could drop formatting altogether, via <<

MarcoFalke commented at 2:07 pm on June 14, 2019:

I believe tfm uses << under the hood, so it shouldn’t matter.

Empact commented at 2:20 pm on June 14, 2019:

Yeah, the output should be the same, just would remove a layer of indirection.

laanwj commented at 1:10 pm on June 15, 2019:

that’s just a matter of style; it seems most of the developers of this code base prefer %X formatting to the various << incantations (hence the use of tinyformat in the first place), so consistently using tfm::format seems good to me

Empact commented at 9:51 pm on June 16, 2019:

Fair enough. Just a nit.

Empact commented at 1:10 pm on June 14, 2019: member

ACK https://github.com/bitcoin/bitcoin/pull/16205/commits/fa8f195195945ce6258199af0461e3fbfbc1236d

laanwj commented at 1:12 pm on June 15, 2019: member

When reviewing please note that tfm::format in contrast to fprintf may throw (tinyformat::format_error). That is not necessarily a problem obviously, but it is worth to have in mind when analysing the change in possible code paths from this change.

Another thing to be careful of is to not use stderr and std::cerr in a mixed way due to potential buffering issues. So it’s essential to do this switch in one go (which is the intent so that’s fine).

laanwj approved

laanwj commented at 1:48 pm on June 15, 2019: member

code review and lightly tested ACK fa8f195195945ce6258199af0461e3fbfbc1236d

in src/sync.cpp:60 in fa8f195195

56@@ -57,7 +57,7 @@ struct CLockLocation {
57 
58     std::string ToString() const
59     {
60-        return tfm::format(
61+        return strprintf(

jonatack commented at 9:27 pm on June 16, 2019:

IIUC this change is only required due to the test/lint/lint-format-strings.sh linter change in fa8f195195945ce6258199af0461e3fbfbc1236d of this PR.

E.g. if the above line is not changed then ./test/lint/lint-format-strings.sh will raise

0src/sync.cpp: Expected 0 argument(s) after format string but found 4 argument(s): tfm::format( "%s %s:%s%s (in thread %s)", mutexName, sourceFile, itostr(sourceLine), (fTry ? " (TRY)" : ""), m_thread_name)

whereas on master it does not raise.

Perhaps an quick note in the commit and PR description explaining why this changed could be helpful.

MarcoFalke commented at 11:10 am on June 17, 2019:

Correct, but I didn’t rewrite the commits to preserve the acks.

jonatack commented at 9:34 pm on June 16, 2019: member

ACK fa8f195195945ce6258199af0461e3fbfbc1236d from light code review, building, and running linter/unit tests/extended functional tests.

MarcoFalke referenced this in commit 1a274bce4b on Jun 17, 2019

MarcoFalke merged this on Jun 17, 2019

MarcoFalke closed this on Jun 17, 2019

MarcoFalke deleted the branch on Jun 17, 2019

kallewoof commented at 1:52 pm on June 17, 2019: member

Post-merge utACK

MarcoFalke commented at 1:18 pm on June 18, 2019: member

Looks like this accidentally fixed multiple test failures with the cross-compiled windows binaries:

feature_config_args:

0Traceback (most recent call last):
1  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_framework.py", line 193, in main
2    self.run_test()
3  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/feature_config_args.py", line 67, in run_test
4    self.test_config_file_parser()
5  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/feature_config_args.py", line 57, in test_config_file_parser
6    self.nodes[0].stop_node(expected_stderr='Warning: ' + inc_conf_file_path + ':1 Section [testnot] is not recognized.' + os.linesep + 'Warning: ' + inc_conf_file2_path + ':1 Section [testnet] is not recognized.')
7  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_node.py", line 276, in stop_node
8    raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
9AssertionError: Unexpected stderr  != Warning: C:\Users\travis\AppData\Local\Temp\test_runner_₿_🏃_20190617_121557\feature_config_args_3\node0\include.conf:1 Section [testnot] is not recognized.

feature_includeconf

0Traceback (most recent call last):
1  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_framework.py", line 193, in main
2    self.run_test()
3  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/feature_includeconf.py", line 55, in run_test
4    self.stop_node(0, expected_stderr="warning: -includeconf cannot be used from included files; ignoring -includeconf=relative2.conf")
5  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_framework.py", line 390, in stop_node
6    self.nodes[i].stop_node(expected_stderr, wait=wait)
7  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_node.py", line 276, in stop_node
8    raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
9AssertionError: Unexpected stderr  != warning: -includeconf cannot be used from included files; ignoring -includeconf=relative2.conf

tool_wallet

 0Traceback (most recent call last):
 1  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_framework.py", line 193, in main
 2    self.run_test()
 3  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/tool_wallet.py", line 61, in run_test
 4    self.assert_tool_output(out, '-wallet=wallet.dat', 'info')
 5  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/tool_wallet.py", line 37, in assert_tool_output
 6    assert_equal(stdout, output)
 7  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\util.py", line 39, in assert_equal
 8    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
 9AssertionError: not(Wallet info
10===========
11Encrypted: no
12HD (hd seed available): yes
13Keypool Size: 2
14Transactions: zu
15Address Book: zu
16 == Wallet info
17===========
18Encrypted: no
19HD (hd seed available): yes
20Keypool Size: 2
21Transactions: 0
22Address Book: 3
23)

MarcoFalke commented at 1:24 pm on June 18, 2019: member

I am going to backport this, since it turned out to be a bugfix

MarcoFalke added the label Utils/log/libs on Jun 18, 2019

MarcoFalke removed the label Refactoring on Jun 18, 2019

MarcoFalke removed this from the milestone 0.19.0 on Jun 18, 2019

MarcoFalke referenced this in commit f88959ba7c on Jun 18, 2019

MarcoFalke referenced this in commit beb09f09b3 on Jun 18, 2019

MarcoFalke referenced this in commit 79745d1752 on Jun 18, 2019

HashUnlimited referenced this in commit 69a6c647f9 on Aug 23, 2019

HashUnlimited referenced this in commit a13beb7712 on Aug 23, 2019

HashUnlimited referenced this in commit 42bb00aa0b on Aug 23, 2019

Bushstar referenced this in commit ae08b726e0 on Aug 24, 2019

Bushstar referenced this in commit 44eeadd235 on Aug 24, 2019

Bushstar referenced this in commit d713694818 on Aug 24, 2019

Bushstar referenced this in commit 17239604ef on Aug 24, 2019

Bushstar referenced this in commit 3629a82724 on Aug 24, 2019

deadalnix referenced this in commit a64cbe0c97 on Apr 20, 2020

UdjinM6 referenced this in commit a8769a3cda on Oct 24, 2021

pravblockc referenced this in commit f91ba51ec0 on Nov 18, 2021

DrahtBot locked this on Dec 16, 2021

Refactor: Replace fprintf with tfm::format #16205

Conflicts