See discussion: https://reviews.bitcoinabc.org/D1425
Details
Details
- Reviewers
deadalnix schancel - Group Reviewers
Restricted Project - Commits
- rSTAGING080890188304: Added error message when logging on categories NONE and ALL
rABC080890188304: Added error message when logging on categories NONE and ALL
make check
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- arcpatch-D1431
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 2745 Build 3601: Bitcoin ABC Buildbot (legacy) Build 3600: arc lint + arc unit
Event Timeline
Comment Actions
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. |
| src/logging.cpp | ||
|---|---|---|
| 215 | 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 | |
| src/logging.cpp | ||
|---|---|---|
| 215 | 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 | 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. | |
| 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. |
Comment Actions
Removed the test, as it's no longer necessary. Also added a newline to the error message.