Page MenuHomePhabricator

Added error message when logging on categories NONE and ALL
ClosedPublic

Authored by jasonbcox on May 20 2018, 23:54.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
arcpatch-D1431
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2837
Build 3778: Bitcoin ABC Buildbot (legacy)
Build 3777: arc lint + arc unit

Event Timeline

Planned changes: Adding a test to make sure the assert() is enforced for all logging methods that accept a category. I'll only write these tests if this change is acceptable to everyone else.

src/logging.cpp
212 ↗(On Diff #3915)

This needs to be a static assert. Can we make the logger a template somehow?

src/logging.cpp
212 ↗(On Diff #3915)

This cannot be a static_assert because category is not a constant expression. The intent here is to prevent calls made using ALL. Is there a better way to do this so that static_assert still works? Conceptually, catching this at compile-time makes sense, but I don't think it can be done in a way that catches all cases.

Not sure exactly what you mean by making the logger a template, but I was talking with jimpo about logging improvements, which he started on here: https://reviews.bitcoinabc.org/D1277 Most of our conversation was in slack I believe. We talked about templating some of the functions, iirc, but I would have to dig through the chat logs.

src/logging.cpp
212 ↗(On Diff #3915)

Having the logger potentially crashing the app seems like a very bad tradeof.

New strategy: log the error, return false, write a test.

jasonbcox retitled this revision from Added assert to prevent logging on category ALL to Added assert to prevent logging on categories NONE and ALL.Jun 15 2018, 17:58
schancel added inline comments.
src/logging.cpp
215 ↗(On Diff #4128)

This seems to cause incorrect log message to not get logged at all, and instead only the above error message is printed. (See: logging.h macro LogPrint)

Please change to true

This revision is now accepted and ready to land.Jun 18 2018, 13:15
schancel requested changes to this revision.Jun 18 2018, 13:17
This revision now requires changes to proceed.Jun 18 2018, 13:17
src/logging.cpp
215 ↗(On Diff #4128)

My original intent was to not log the original message so the caller is forced to fix it, otherwise the error message can be ignored.

src/logging.cpp
215 ↗(On Diff #4128)

I understand, but I still disagree. A build which uses this should hopefully never make it into production, but we don't have any linters yet. If it did make it into a production release, and we needed the log message for something -- not having it would be really frustrating.

jasonbcox retitled this revision from Added assert to prevent logging on categories NONE and ALL to Added error message when logging on categories NONE and ALL.Jun 25 2018, 16:49
jasonbcox marked 3 inline comments as done.

Allow original log message per feedback.

deadalnix requested changes to this revision.Jun 30 2018, 15:11
deadalnix added inline comments.
src/logging.cpp
214 ↗(On Diff #4158)

Should this use a new line at the end ?

src/test/logging_tests.cpp
15 ↗(On Diff #4158)

Why not zero ?

23 ↗(On Diff #4158)

I'm not sure what this test is checking if it is not playing with logCategories . Right now everything expect true, so it's clearly nto testing what the function is doing.

This revision now requires changes to proceed.Jun 30 2018, 15:11
jasonbcox added inline comments.
src/test/logging_tests.cpp
15 ↗(On Diff #4158)

NONE is 0. I guess since all of the logs should log, I can just fold them into this loop now.

23 ↗(On Diff #4158)

Maybe I should remove the test all together? Logging the error probably doesn't need a test.

Removed the test, as it's no longer necessary. Also added a newline to the error message.

This revision is now accepted and ready to land.Jul 14 2018, 23:56
This revision was automatically updated to reflect the committed changes.