Page MenuHomePhabricator

Remove LogPrintf, use LogPrint everywhere instead
AbandonedPublic

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 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)
schancel retitled this revision from Remove LogPrintf, use LogPrint everywhere insteadCompletes T214 to Remove LogPrintf, use LogPrint everywhere instead.Nov 7 2018, 00:36
schancel requested changes to this revision.Nov 7 2018, 00:39
schancel added a subscriber: schancel.

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

Fabien marked 3 inline comments as done.Nov 7 2018, 09:54

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

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 ?

deadalnix requested changes to this revision.Nov 7 2018, 23:17
deadalnix added a subscriber: deadalnix.

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

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.