Skip to content

Conversation

thelazier
Copy link
Contributor

Printing Log without category defined should use LogPrintf

Printing Log without category defined should use LogPrintf
@eduffield222
Copy link

utACK

@pstratem
Copy link
Contributor

utACK ba61949

@laanwj
Copy link
Member

laanwj commented Jun 21, 2016

utACK ba61949
Good catch!
This mistake is all too easy to make, unfortunately.

At some point we either need to rename one of those functions, or create an automatic checker for Printf/LogPrintf arguments, or both.

@laanwj laanwj merged commit ba61949 into bitcoin:0.12 Jun 21, 2016
laanwj added a commit that referenced this pull request Jun 21, 2016
ba61949 Fix LogPrint to LogPrintf (TheLazieR Yip)
@gmaxwell
Copy link
Contributor

I hate that this isn't typesafe, what is this, PHP? :) ... when I saw the ticket open I felt immediately stupid, sure than it must have been me that did this again. (posthumous utACK).

@laanwj
Copy link
Member

laanwj commented Jun 21, 2016

Eh, why is this merging into 0.12 though?
Isn't this a problem on master?

I hate that this isn't typesafe, what is this, PHP? :) .

Well it was a step up from C's printf - none of this will allow mangling values or corrupting memory.
It is typesafe, e.g you don't need size specifiers, it doesn't matter if you use %i or %lli etc.
Worst-case, you get an exception if the number of arguments doesn't match the number of %. Or nothing is logged and you rub your head why.
There would be logging methods that are more robust, e.g. C++/Qt style:

LogInfo() << "";
LogDebug("rpc") << "Invalid RPC call " << strMethod;

(tinyformat even uses this internally, but emulates C printf-isms because that was what was already in the code and this had the least impact)

Then again, there's so much to do already, it's never been urgent.

laanwj pushed a commit that referenced this pull request Jun 21, 2016
Printing Log without category defined should use LogPrintf

Github-Pull: #8230
Meta: PR should have been based on master in the first place
@laanwj
Copy link
Member

laanwj commented Jun 21, 2016

Forward-ported to master as bf9c70b.
(this is not how we usually work, please base your PR on master next time unless it's a backport)

@sipa
Copy link
Member

sipa commented Jun 21, 2016 via email

@laanwj
Copy link
Member

laanwj commented Jun 21, 2016

Changing the debug category into an enum would also avoid type ambiguity

That's a neat idea, a problem with that is that it requires centralizing all the debug categories of the program in one place. That would be inimical to modularization.

Another option would be to create a DebugCategory type that is based on string, and only allow passing that as category argument.

DebugCategory LOG_RPC("rpc");
...
LogPrintf(LOG_RPC, "Invalid RPC call %s\n", strMethod);

@thelazier thelazier deleted the patch-1 branch June 21, 2016 14:11
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants