Refactor: Replace fprintf with tfm::format by maflcko · Pull Request #16205 · bitcoin/bitcoin

maflcko · 2019-06-13T13:34:22Z

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

laanwj · 2019-06-13T13:47:31Z

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 · 2019-06-13T13:51:55Z

This might do it:

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

-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

practicalswift · 2019-06-13T14:43:36Z

Concept ACK

DrahtBot · 2019-06-13T14:49:58Z

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 · 2019-06-13T15:32:03Z

Concept ACK, will test in a bit.

promag · 2019-06-13T21:43:31Z

ACK fa8f195. Ideally this should be rebased before merge.

practicalswift · 2019-06-13T21:51:57Z

utACK fa8f195

practicalswift · 2019-06-14T08:03:58Z

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.

Empact · 2019-06-14T13:01:19Z

src/bitcoin-tx.cpp


        if (argc < 2) {
-            fprintf(stderr, "Error: too few parameters\n");
+            tfm::format(std::cerr, "Error: too few parameters\n");


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

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

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

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

Fair enough. Just a nit.

Empact · 2019-06-14T13:10:44Z

ACK fa8f195

laanwj · 2019-06-15T13:12:04Z

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

code review and lightly tested ACK fa8f195

jonatack

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

jonatack · 2019-06-16T21:27:09Z

src/sync.cpp

    std::string ToString() const
    {
-        return tfm::format(
+        return strprintf(


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

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

src/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.

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

fa8f195 Replace remaining fprintf with tfm::format manually (MarcoFalke) fac03ec scripted-diff: Replace fprintf with tfm::format (MarcoFalke) fa72a64 tinyformat: Add doc to Bitcoin Core specific strprintf (MarcoFalke) Pull request description: 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 bitcoin#8730 ACKs for commit fa8f19: promag: ACK fa8f195. Ideally this should be rebased before merge. practicalswift: utACK fa8f195 Empact: ACK bitcoin@fa8f195 laanwj: code review and lightly tested ACK fa8f195 jonatack: ACK fa8f195 from light code review, building, and running linter/unit tests/extended functional tests. Tree-SHA512: 65f648b0bc383e3266a5bdb4ad8c8a1908a719635d49e1cd321b91254be24dbc7e22290370178e29b98ddcb3fec0889de9cbae273c7140abc9793d849534a743

kallewoof · 2019-06-17T13:52:51Z

Post-merge utACK

maflcko · 2019-06-18T13:18:28Z

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

feature_config_args:

Traceback (most recent call last):
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_framework.py", line 193, in main
    self.run_test()
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/feature_config_args.py", line 67, in run_test
    self.test_config_file_parser()
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/feature_config_args.py", line 57, in test_config_file_parser
    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.')
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_node.py", line 276, in stop_node
    raise AssertionError("Unexpected stderr {} != {}".format(stderr, expected_stderr))
AssertionError: 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

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

tool_wallet

Traceback (most recent call last):
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\test_framework.py", line 193, in main
    self.run_test()
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/tool_wallet.py", line 61, in run_test
    self.assert_tool_output(out, '-wallet=wallet.dat', 'info')
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\/test/functional/tool_wallet.py", line 37, in assert_tool_output
    assert_equal(stdout, output)
  File "C:\Users\travis\build\MarcoFalke\btc_nightly\bitcoin\test\functional\test_framework\util.py", line 39, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(Wallet info
===========
Encrypted: no
HD (hd seed available): yes
Keypool Size: 2
Transactions: zu
Address Book: zu
 == Wallet info
===========
Encrypted: no
HD (hd seed available): yes
Keypool Size: 2
Transactions: 0
Address Book: 3
)

maflcko · 2019-06-18T13:24:21Z

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

Github-Pull: bitcoin#16205 Rebased-From: fa72a64

-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 Github-Pull: bitcoin#16205 Rebased-From: fac03ec

Github-Pull: bitcoin#16205 Rebased-From: fa8f195

Github-Pull: bitcoin#16205 Rebased-From: fa72a64

-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 Github-Pull: bitcoin#16205 Rebased-From: fac03ec

Github-Pull: bitcoin#16205 Rebased-From: fa8f195

Github-Pull: bitcoin#16205 Rebased-From: fa72a64

Github-Pull: bitcoin#16205 Rebased-From: fac03ec

Summary: * tinyformat: Add doc to Bitcoin Core specific strprintf * 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 * Replace remaining fprintf with tfm::format manually This is a backport of Core [[bitcoin/bitcoin#16205 | PR16205]] Test Plan: grep -r ../src/ -e fprintf | grep -v leveldb | grep -v secp256k1 | grep -v univalue Check that there are no mre unexpected callsites. ninja all check-all arc lint --everything Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D5782

* tinyformat: Add doc to Bitcoin Core specific strprintf * 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- * Replace remaining fprintf with tfm::format manually * fixes Co-authored-by: MarcoFalke <[email protected]>

…pay#4531) * tinyformat: Add doc to Bitcoin Core specific strprintf * 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- * Replace remaining fprintf with tfm::format manually * fixes Co-authored-by: MarcoFalke <[email protected]>

tinyformat: Add doc to Bitcoin Core specific strprintf

fa72a64

maflcko added the Refactoring label Jun 13, 2019

maflcko added this to the 0.19.0 milestone Jun 13, 2019

maflcko force-pushed the 1906-nofprintf branch from fa597b3 to fab525a Compare June 13, 2019 13:40

maflcko force-pushed the 1906-nofprintf branch 2 times, most recently from fa517dc to fa6bca6 Compare June 13, 2019 14:00

maflcko force-pushed the 1906-nofprintf branch from fa6bca6 to fa779ef Compare June 13, 2019 14:33

Replace remaining fprintf with tfm::format manually

fa8f195

maflcko force-pushed the 1906-nofprintf branch from fa779ef to fa8f195 Compare June 13, 2019 15:46

Empact reviewed Jun 14, 2019

View reviewed changes

laanwj approved these changes Jun 15, 2019

View reviewed changes

jonatack reviewed Jun 16, 2019

View reviewed changes

maflcko merged commit fa8f195 into bitcoin:master Jun 17, 2019

maflcko deleted the 1906-nofprintf branch June 17, 2019 10:12

hebasto mentioned this pull request Jun 17, 2019

Fix datadir handling #15864

Merged

maflcko added Utils/log/libs and removed Refactoring labels Jun 18, 2019

maflcko removed this from the 0.19.0 milestone Jun 18, 2019

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jun 18, 2019

tinyformat: Add doc to Bitcoin Core specific strprintf

f88959b

Github-Pull: bitcoin#16205 Rebased-From: fa72a64

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Jun 18, 2019

Replace remaining fprintf with tfm::format manually

79745d1

Github-Pull: bitcoin#16205 Rebased-From: fa8f195

practicalswift mentioned this pull request Jun 23, 2019

0.18.1: Backports #16035

Merged

maflcko mentioned this pull request Aug 2, 2019

test: feature_config_args and feature_includeconf fail on native windows #15542

Closed

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 23, 2019

tinyformat: Add doc to Bitcoin Core specific strprintf

69a6c64

Github-Pull: bitcoin#16205 Rebased-From: fa72a64

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 23, 2019

Replace remaining fprintf with tfm::format manually

42bb00a

Github-Pull: bitcoin#16205 Rebased-From: fa8f195

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Aug 24, 2019

tinyformat: Add doc to Bitcoin Core specific strprintf

ae08b72

Github-Pull: bitcoin#16205 Rebased-From: fa72a64

Bushstar pushed a commit to Bushstar/omnicore that referenced this pull request Aug 24, 2019

Replace fprintf with tfm::format

3629a82

Github-Pull: bitcoin#16205 Rebased-From: fac03ec

barton2526 mentioned this pull request Aug 4, 2021

refactor: Replace fprintf with tfm::format gridcoin-community/Gridcoin-Research#2262

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Conversation

maflcko commented Jun 13, 2019

Uh oh!

laanwj commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 13, 2019

Uh oh!

practicalswift commented Jun 13, 2019

Uh oh!

DrahtBot commented Jun 13, 2019

Conflicts

Uh oh!

jamesob commented Jun 13, 2019

Uh oh!

promag commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jun 13, 2019

Uh oh!

practicalswift commented Jun 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

Empact Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj Jun 15, 2019

Choose a reason for hiding this comment

Uh oh!

Empact Jun 16, 2019

Choose a reason for hiding this comment

Uh oh!

Empact commented Jun 14, 2019

Uh oh!

laanwj commented Jun 15, 2019

Uh oh!

laanwj left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Jun 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

kallewoof commented Jun 17, 2019

Uh oh!

maflcko commented Jun 18, 2019

Uh oh!

maflcko commented Jun 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

laanwj commented Jun 13, 2019 •

edited

Loading

promag commented Jun 13, 2019 •

edited

Loading

practicalswift commented Jun 14, 2019 •

edited

Loading

jonatack Jun 16, 2019 •

edited

Loading