Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure LogPrintf strings are line-terminated #6497

Merged
merged 1 commit into from Aug 3, 2015

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Jul 31, 2015

Fix the cases where LogPrint[f] was accidentally called without line terminator, which resulted in concatenated log lines.

(see e.g. #6492)

@laanwj laanwj added the Docs label Jul 31, 2015
@sipa
Copy link
Member

sipa commented Jul 31, 2015

ACK

}
LogPrint("estimatefee", "\n");
Copy link
Member

@morcos morcos Jul 31, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be ugly, but it was working correctly before right? This will cause extra new lines I think, because NewTx above prints a new line. I guess we could just remove it from there. This may be a case of too much debugging, I'm not sure if we want all these anyway....

Copy link
Member Author

@laanwj laanwj Aug 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Original code:

LogPrint("estimatefee", "Blockpolicy mempool tx %s ", hash.ToString().substr(0,10));
 // Record this as a priority estimate
if (entry.GetFee() == 0 || isPriDataPoint(feeRate, curPri)) {
...
}
// Record this as a fee estimate
else if (isFeeDataPoint(feeRate, curPri)) {
...
}
else {
    LogPrint("estimatefee", "not adding\n");
}

So the line will not be terminated unless the if() condition falls through.

But if you insist I'll remove this change...

Copy link
Member

@morcos morcos Aug 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize for the ugly code again, but in the if and else if, feeStats and priStats both call NewTx respectively. NewTx adds a new line. I'd suggest leaving your change, but removing the new line in the LogPrint call in NewTx. Or, removing all the LogPrint lines altogether, what are your thoughts about the level of debugging? I found this useful in writing this code and for certain modifications it would be also valuable, but perhaps doesn't have much everyday value. I'm running into the same questions in the mempool limiting code where debugging the various failures is valuable when working on the code but perhaps not when its finished until someone wants to change it.

Copy link
Member Author

@laanwj laanwj Aug 3, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, will do that.
This is optional debugging, so I think having it in the first place is fine. It can be useful if someone wants to troubleshoot the fee estimation, which I'm sure has to happen at some point.

@jgarzik
Copy link
Contributor

jgarzik commented Aug 1, 2015

ACK though agree @morcos has a point

@Diapolo
Copy link

Diapolo commented Aug 3, 2015

utACK for trivial-branch

Fix the cases where LogPrint[f] was accidentally called without line
terminator, which resulted in concatenated log lines.

(see e.g. bitcoin#6492)
@laanwj laanwj force-pushed the 2015_07_terminate_logprintf branch from 5d7fc47 to f18b8ec Compare Aug 3, 2015
@laanwj
Copy link
Member Author

laanwj commented Aug 3, 2015

Ok, updated for @morcos' comment

@laanwj laanwj merged commit f18b8ec into bitcoin:master Aug 3, 2015
laanwj added a commit that referenced this pull request Aug 3, 2015
f18b8ec Make sure LogPrintf strings are line-terminated (Wladimir J. van der Laan)
@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.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants