- as the redefiniton of printf happens in util.h, which is included in bitcoinrpc.cpp, we don't need another redefinition
remove printf redefinition from bitcoinrpc.cpp #1977
pull Diapolo wants to merge 1 commits into bitcoin:master from Diapolo:rem_printf_redef_rpc changing 1 files +0 −3-
Diapolo commented at 10:26 AM on November 4, 2012: none
-
ed552cfae0
remove printf redefinition from bitcoinrpc.cpp
- as the redefiniton of printf happens in util.h, which is included in bitcoinrpc.cpp, we don't need another redefinition
-
sipa commented at 10:36 AM on November 4, 2012: member
Are you sure this is a no-op? Maybe one of the included header files between the undef and the define needs the original, or tries to define printf by itself?
- sipa closed this on Nov 4, 2012
- sipa reopened this on Nov 4, 2012
-
Diapolo commented at 10:44 AM on November 4, 2012: none
It was this pull, who included this change https://github.com/bitcoin/bitcoin/commit/69d605f410c83235aa7b757445e7d0166fcfe2d9, but as you see between the #undef and the #define are only boost or std headers, well I'm not sure it's a no-op, but compilation and running the client is working fine. We include util.h quite often and it's mostly before boost headers... @laanwj It was your pull, perhaps you can enlighten this?
-
laanwj commented at 11:03 AM on November 4, 2012: member
That line is actually ancient.
It's showing up the text as being from me because I renamed
src/rpc.cpptosrc/bitcoinrpc.cpp. However, the line was in there way before that. Some deeper git forensics shows that the first commit in which the printf redefinition was insrc/rpc.cppwas 84c3fb0. This appears to be also a reorganization commit, in which/rpc.cppwas renamed tosrc/rpc.cpp. Digging even deeper, 22f721d was really the first commit in which it was introduced (by Satoshi himself).Looks like it was added for "MSVC 8 compatibility".
FYI, the git command to grep a while throughout all commits:
git grep OutputDebugStringF $(git log --pretty=format:%h) -- rpc.cpp -
Diapolo commented at 11:08 AM on November 4, 2012: none
Thanks for digging, although I'm still not sure if it's needed there or not. MSVC 8 is ancient, also.
-
laanwj commented at 11:12 AM on November 4, 2012: member
No one knows. I'm ok with removing it. After all, as you say, all the rpc*.cpp spinoffs work fine without it.
Though my real preference would still be to remove the re-definition of printf, rename OutputDebugStringF to some sane name such as debug_log(), and use that throughout the source code. But I don't think everyone agrees.
-
BitcoinPullTester commented at 4:53 PM on November 4, 2012: none
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/ed552cfae078778ebaebf1786bad0305ee730479 for binaries and test log.
-
sipa commented at 12:43 AM on November 5, 2012: member
ACK
- laanwj referenced this in commit e88f8887b6 on Nov 10, 2012
- laanwj merged this on Nov 10, 2012
- laanwj closed this on Nov 10, 2012
- laudney referenced this in commit 3cdda4b34c on Mar 19, 2014
- KolbyML referenced this in commit bc2ec8590f on Dec 5, 2020
- DrahtBot locked this on Sep 8, 2021