Details
- Reviewers
deadalnix jasonbcox jimpo - Group Reviewers
Restricted Project - Commits
- rSTAGINGd0fb55cdf701: Ensure checks are enforced in core_read.cpp for all push types
rABCd0fb55cdf701: Ensure checks are enforced in core_read.cpp for all push types
make check
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- arcpatch-D1295
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 2413 Build 2955: Bitcoin ABC Buildbot (legacy) Build 2954: arc lint + arc unit
Event Timeline
| src/core_read.cpp | ||
|---|---|---|
| 128 ↗ | (On Diff #3632) | It's not clear what that comment is doing here. Also, rather than explaining what something is not, it's probably a better idea to explain what it is. |
| 168 ↗ | (On Diff #3632) | I'd move that part after the block line 122, so we get a clear separation between data and instructions. In addition, some more comments would be nice :) |
| src/test/core_io_tests.cpp | ||
| 1018 ↗ | (On Diff #3632) | You should consider building these programatically. |
| src/core_read.cpp | ||
|---|---|---|
| 52 | There is no reason to predeclare size_change. | |
| 126 | Explain what each block is for in comments. | |
| 129 | ??? | |
| 131 | Describe what it is rather than what it is not. What it is not is not helpful. | |
| 145 | Comment what this block is for. | |
| src/test/core_io_tests.cpp | ||
| 37 | PrintLE can be unit tested easily. | |
| 64 | This can do with a test, as it is really easy and would ensure this test what we think it test. | |
| 88 | This is inaccurate. It depends on flags passed down. Please don't assume things. | |
| 91 | This checks for something too large, but not something too small. | |
| 97 | Invalid size's size must also be tested. | |
| 111 | It doesn't work because 'help' is 5 bytes. There is no check that disallow this. | |
| src/core_read.cpp | ||
|---|---|---|
| 129 | Leftover assert from when the code was laid out differently. | |
| 128 ↗ | (On Diff #3632) | Shouldn't be there. I need to fix my comment-blindness so I stop leaving them places they don't belong. |
| src/test/core_io_tests.cpp | ||
| 111 | You're right. There was before though. You had me remove it. I didn't notice it was wrong now. | |
A few nits, but the logic looks good.
| src/core_read.cpp | ||
|---|---|---|
| 52 ↗ | (On Diff #3709) | There is no reason to predeclare size_change as its use is scoped. |
| 148 ↗ | (On Diff #3709) | It can also be in the form of the opcode's name. |
| src/test/core_io_tests.cpp | ||
| 111 ↗ | (On Diff #3709) | This can be moved in its own test case, also doesn't check various length. |
| 114 ↗ | (On Diff #3709) | dito |
| src/core_read.cpp | ||
|---|---|---|
| 119 ↗ | (On Diff #3724) | I don't really get why this needs to be moved. It seems to me like the appropriate place is at line 66 as we are effectively doing a sort of bucket brigade here with these two variables. |