Remove LogPrintf, use LogPrint everywhere instead
AbandonedPublic
Actions

Authored by Fabien on Nov 7 2018, 00:26.

Details

Reviewers

jasonbcox
schancel
deadalnix

Group Reviewers

Restricted Project

Summary

Completes T214

Test Plan

make check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

LogPrintf_migration

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 3804
Build 5682: Bitcoin ABC Buildbot (legacy)
Build 5681: arc lint + arc unit

Event Timeline

Fabien created this revision.Nov 7 2018, 00:26

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Fabien retitled this revision from Remove LogPrintf, use LogPrint everywhere instead Completes T214 to Remove LogPrintf, use LogPrint everywhere insteadCompletes T214.Nov 7 2018, 00:29

Fabien edited the summary of this revision. (Show Details)

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B3799: Diff 5686.Nov 7 2018, 00:34

schancel retitled this revision from Remove LogPrintf, use LogPrint everywhere insteadCompletes T214 to Remove LogPrintf, use LogPrint everywhere instead.Nov 7 2018, 00:36

This is great. However we need to introduce a LogPrintLn, and a LogPrint. LogPrintLn should print a newline included, LogPrint should not. There's multiple log places which used LogPrintf to be able to print a single log line in multiple calls. (or maybe it was the other way around... I don't remember which one prints the newline and which doesn't)

This revision now requires changes to proceed.Nov 7 2018, 00:39

schancel awarded a token.Nov 7 2018, 00:39

jasonbcox requested changes to this revision.Nov 7 2018, 00:46

jasonbcox added inline comments.

src/logging.cpp
231 ↗	(On Diff #5686)	Is it possible to keep this const and make LogPrintStr const as well? If it's too complex, maybe reserve this change for another diff in the future.
src/logging.h
35 ↗	(On Diff #5686)	This diff is already quite large. Please make this change in another diff.
56 ↗	(On Diff #5686)	Thanks for adding these. Please double check that latest Core hasn't reserved any of these so that we do not collide during backports.

I don't think there was a difference between LogPrint and LogPrintf regarding the newline, both ended up calling the same LogPrintStr method, and \n is added at almost every call.
However this can be a good improvement for another diff.

src/logging.cpp
231 ↗	(On Diff #5686)	I will try to make it const again, and will keep it in this diff it it does not change to many other things
src/logging.h
35 ↗	(On Diff #5686)	I will move it to another diff
56 ↗	(On Diff #5686)	Checked, core has no additional flag

Renaming BD->WALLETDB will be moved to another diff

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster completed remote builds in B3804: Diff 5692.Nov 7 2018, 15:46

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Fabien marked an inline comment as done.Nov 7 2018, 15:51

Fabien added inline comments.

src/logging.cpp
231	It will require more change than I expected to make this const again, because the LogPrintStr method needs to modify some members (buffer, file descriptor, ...). The way it worked in the previous version was not very elegant : LogPrintf is a macro that call GetLogger().LogPrintStr(). One solution may be to move the if(...) LogPrintStr section to the caller function `LogAcceptCategory` but I find it makes the code more fragmented and more difficult to follow. Any thought ?

Fabien added a child revision: D2019: Rename BCLog::DB to BCLog::WALLETDB.Nov 7 2018, 16:39

This makes zero sense. There is no point debating details on a diff that huge, there isn't even an agreement on what needs to be done.

This revision now requires changes to proceed.Nov 7 2018, 23:17

In D2016#43578, @deadalnix wrote:

This makes zero sense. There is no point debating details on a diff that huge, there isn't even an agreement on what needs to be done.

See the task: https://reviews.bitcoinabc.org/T214 The idea is that all log messages should have a category. If that category must be "always log this no matter what", then we should have an ALWAYS_LOG or IMPORTANT category.

But I do agree, this should be done in small diffs. One file or groups of related files at a time would be best.

Fabien abandoned this revision.Dec 21 2018, 16:14