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
- op_pushdata_tests
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 2347 Build 2829: Bitcoin ABC Buildbot (legacy) Build 2828: 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 ↗ | (On Diff #3643) | There is no reason to predeclare size_change. |
| 126 ↗ | (On Diff #3643) | Explain what each block is for in comments. |
| 129 ↗ | (On Diff #3643) | ??? |
| 131 ↗ | (On Diff #3643) | Describe what it is rather than what it is not. What it is not is not helpful. |
| 145 ↗ | (On Diff #3643) | Comment what this block is for. |
| src/test/core_io_tests.cpp | ||
| 37 ↗ | (On Diff #3643) | PrintLE can be unit tested easily. |
| 64 ↗ | (On Diff #3643) | This can do with a test, as it is really easy and would ensure this test what we think it test. |
| 88 ↗ | (On Diff #3643) | This is inaccurate. It depends on flags passed down. Please don't assume things. |
| 91 ↗ | (On Diff #3643) | This checks for something too large, but not something too small. |
| 97 ↗ | (On Diff #3643) | Invalid size's size must also be tested. |
| 111 ↗ | (On Diff #3643) | It doesn't work because 'help' is 5 bytes. There is no check that disallow this. |
| src/core_read.cpp | ||
|---|---|---|
| 129 ↗ | (On Diff #3643) | 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 ↗ | (On Diff #3643) | 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. |