remove printf redefinition from bitcoinrpc.cpp #1977

Merged
merged 1 commit into from Nov 10, 2012

Conversation

Projects
None yet
4 participants
@Diapolo

Diapolo commented Nov 4, 2012

  • as the redefiniton of printf happens in util.h, which is included in
    bitcoinrpc.cpp, we don't need another redefinition
Philip Kaufmann
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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 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?

Member

sipa commented Nov 4, 2012

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 sipa closed this Nov 4, 2012

@sipa sipa reopened this Nov 4, 2012

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Nov 4, 2012

It was this pull, who included this change 69d605f, 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?

Diapolo commented Nov 4, 2012

It was this pull, who included this change 69d605f, 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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 4, 2012

Member

That line is actually ancient.

It's showing up the text as being from me because I renamed src/rpc.cpp to src/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 in src/rpc.cpp was 84c3fb0. This appears to be also a reorganization commit, in which /rpc.cpp was renamed to src/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
Member

laanwj commented Nov 4, 2012

That line is actually ancient.

It's showing up the text as being from me because I renamed src/rpc.cpp to src/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 in src/rpc.cpp was 84c3fb0. This appears to be also a reorganization commit, in which /rpc.cpp was renamed to src/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

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Nov 4, 2012

Thanks for digging, although I'm still not sure if it's needed there or not. MSVC 8 is ancient, also.

Diapolo commented Nov 4, 2012

Thanks for digging, although I'm still not sure if it's needed there or not. MSVC 8 is ancient, also.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 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.

Member

laanwj commented Nov 4, 2012

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

This comment has been minimized.

Show comment
Hide comment

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/ed552cfae078778ebaebf1786bad0305ee730479 for binaries and test log.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 5, 2012

Member

ACK

Member

sipa commented Nov 5, 2012

ACK

laanwj added a commit that referenced this pull request Nov 10, 2012

Merge pull request #1977 from Diapolo/rem_printf_redef_rpc
remove printf redefinition from bitcoinrpc.cpp

@laanwj laanwj merged commit e88f888 into bitcoin:master Nov 10, 2012

laudney pushed a commit to reddcoin-project/reddcoin that referenced this pull request Mar 19, 2014

Merge pull request #1977 from Diapolo/rem_printf_redef_rpc
remove printf redefinition from bitcoinrpc.cpp

Duality-CDOO pushed a commit to Duality-CDOO/bitcoin that referenced this pull request Jun 14, 2018

Fix some (potential dead)locks (#1977)
* Avoid locking cs_main in CMasternode/Ping/Broadcast

Deligate this to CMasternodeMan and use AssertLockHeld instead

* Ensure consistent locking order (cs_main, cs_wallet, mempool.cs, cs_instantsend) in CInstantSend while avoiding potential deadlocks at the same time

* Add missing locks in wallet

* Add missing locks in governance

* Fix cs_vPendingMasternodes vs cs_main potential deadlock

SendVerifyRequest no longer opens connection directly, so no cs_main is needed here

Duality-CDOO pushed a commit to Duality-CDOO/bitcoin that referenced this pull request Jun 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment