Page MenuHomePhabricator
Differential D2797

streams: Create VectorReader stream interface for vectors.
ClosedPublic
Actions

Authored by markblundeberg on Apr 12 2019, 02:00.

Details

Reviewers
deadalnix
Fabien
Group Reviewers
Restricted Project
Maniphest Tasks
T589: BIP157 & BIP158 backports
Commits
rSTAGING6c25e0941fc8: streams: Create VectorReader stream interface for vectors.
rABC6c25e0941fc8: streams: Create VectorReader stream interface for vectors.
Summary

This is a read analogue for the existing CVectorWriter.

For BIP158; is cherry-picked from two commits (947133d and 87f2d9e) from:
https://github.com/bitcoin/bitcoin/pull/12254/commits

In addition, the buggy seek() method was removed per later backport:
https://github.com/bitcoin/bitcoin/pull/14357

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
bip158a
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5439
Build 8940: Bitcoin ABC Buildbot (legacy)
Build 8939: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Apr 12 2019, 02:00
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 12 2019, 02:00
Herald added a reviewer: deadalnix. · View Herald Transcript
teamcity added a comment.Apr 12 2019, 02:01

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

markblundeberg added a child revision: D2798: streams: Unit test for VectorReader class..Apr 12 2019, 02:02
markblundeberg added a task: T589: BIP157 & BIP158 backports.Apr 12 2019, 02:03
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg edited the summary of this revision. (Show Details)Apr 12 2019, 02:11
markblundeberg edited the test plan for this revision. (Show Details)
markblundeberg planned changes to this revision.Apr 12 2019, 02:14
teamcity added a comment.Apr 12 2019, 02:15

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5439: Diff 8038.Apr 12 2019, 02:15
markblundeberg updated this revision to Diff 8040.Apr 12 2019, 02:36
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg edited the test plan for this revision. (Show Details)

squashed in child commit

teamcity added a comment.Apr 12 2019, 02:37

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Apr 12 2019, 02:51

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5441: Diff 8040.Apr 12 2019, 02:51
markblundeberg edited the summary of this revision. (Show Details)Apr 12 2019, 02:51
markblundeberg added a child revision: D2799: streams: Implement BitStreamReader/Writer classes..Apr 12 2019, 03:16
Fabien requested changes to this revision.Apr 12 2019, 09:39
Fabien added a subscriber: Fabien.
Fabien added inline comments.
src/streams.h
124 ↗(On Diff #8040)
/**
 * Minimal stream for reading from an existing vector by reference
 */
138 ↗(On Diff #8040)

This comment block contains doxygen tags, it should be a doxygen block.

148 ↗(On Diff #8040)

Dito (even if the (other params same as above) part will make no sense in a doxygen html output).

183 ↗(On Diff #8040)

I find it weird that the position gets changed to something out of bound even if the exception is thrown.
The behavior of the read() function seems more logical to me. Thought ?

src/test/streams_tests.cpp
99 ↗(On Diff #8040)

Move comment above.

110 ↗(On Diff #8040)

Dito.

This revision now requires changes to proceed.Apr 12 2019, 09:39
deadalnix requested changes to this revision.Apr 12 2019, 11:11
deadalnix added inline comments.
src/test/streams_tests.cpp
76 ↗(On Diff #8040)

Use types from cstdint.

@Fabien that would make for a good linter that can auto-correct.

markblundeberg planned changes to this revision.Apr 12 2019, 15:45
markblundeberg added inline comments.
src/streams.h
183 ↗(On Diff #8040)

I agree; looks like this seek() method got abandoned in a later commit anyway. (I'm gonna have to be more careful with the git blame forward-looking :-) ) https://github.com/bitcoin/bitcoin/commit/958e1a307e92f6678459ebdcd64e05b61f5f3a77#diff-eb3b977a68473a9d20093cbe90c659e6

markblundeberg marked 7 inline comments as done.Apr 12 2019, 19:14
markblundeberg updated this revision to Diff 8043.Apr 12 2019, 19:37
markblundeberg edited the summary of this revision. (Show Details)
  • remove seek method per later backport
  • adjust comments style per comments
  • use cstdint types
teamcity added a comment.Apr 12 2019, 19:38

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.Apr 12 2019, 19:52

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5444: Diff 8043.Apr 12 2019, 19:52
deadalnix accepted this revision.Apr 14 2019, 19:37
Fabien accepted this revision.Apr 16 2019, 07:20
This revision is now accepted and ready to land.Apr 16 2019, 07:20
Closed by commit rABC6c25e0941fc8: streams: Create VectorReader stream interface for vectors. (authored by jimpo, committed by Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com>). · Explain WhyApr 16 2019, 22:35
This revision was automatically updated to reflect the committed changes.
Mark Lundeberg <36528214+markblundeberg@users.noreply.github.com> added a commit: rSTAGING6c25e0941fc8: streams: Create VectorReader stream interface for vectors..Sep 18 2019, 22:59

Revision Contents
Changeset List

PathSize
src/
67 lines

Diff 8038

View Options

src/streams.h

Loading...