Page MenuHomePhabricator

streams: Create VectorReader stream interface for vectors.
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markblundeberg created this revision.Fri, Apr 12, 02:00
Herald added a reviewer: Restricted Project. · View Herald TranscriptFri, Apr 12, 02:00
markblundeberg edited the summary of this revision. (Show Details)Fri, Apr 12, 02:11
markblundeberg edited the test plan for this revision. (Show Details)
markblundeberg planned changes to this revision.Fri, Apr 12, 02:14
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg edited the test plan for this revision. (Show Details)

squashed in child commit

markblundeberg edited the summary of this revision. (Show Details)Fri, Apr 12, 02:51
Fabien requested changes to this revision.Fri, Apr 12, 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.Fri, Apr 12, 09:39
deadalnix requested changes to this revision.Fri, Apr 12, 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.Fri, Apr 12, 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.Fri, Apr 12, 19:14
markblundeberg edited the summary of this revision. (Show Details)
  • remove seek method per later backport
  • adjust comments style per comments
  • use cstdint types
deadalnix accepted this revision.Sun, Apr 14, 19:37
Fabien accepted this revision.Tue, Apr 16, 07:20
This revision is now accepted and ready to land.Tue, Apr 16, 07:20
This revision was automatically updated to reflect the committed changes.